Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Aamohd/file based config #662

Merged
merged 1 commit into from
Jan 10, 2025
Merged

Aamohd/file based config #662

merged 1 commit into from
Jan 10, 2025

Conversation

aamohd
Copy link
Contributor

@aamohd aamohd commented Nov 25, 2024

No description provided.

@aamohd aamohd force-pushed the aamohd/file-based-config branch 3 times, most recently from 3c7b075 to 866ea2b Compare December 9, 2024 18:49
@j-lanson j-lanson added type: enhancement New feature or request type: ui UI-related changes that should get heightened review. product: hc Relates to the core "hc" binary labels Dec 9, 2024
hipcheck/src/cli.rs Outdated Show resolved Hide resolved
hipcheck/src/cli.rs Outdated Show resolved Hide resolved
hipcheck/src/executor.rs Outdated Show resolved Hide resolved
hipcheck/src/main.rs Outdated Show resolved Hide resolved
hipcheck/src/plugin/types.rs Outdated Show resolved Hide resolved
@j-lanson
Copy link
Collaborator

j-lanson commented Dec 9, 2024

As I mentioned in one of the requested changes, we shouldn't take./config/Exec.kdl as the backup location for the exec file, it should just be Exec.kdl. But in git version control we are storing the file at ./config/Exec.kdl, which means people would need to pass --exec every time they want to run Hipcheck.

I think the solution is that everyone developing on Hipcheck copies ./config/Exec.kdl to ./Exec.kdl at the project root so it gets picked up as the default location, and this will be everyone's personal Exec.kdl file that doesn't get tracked in git. We can also add a rule to the .gitignore to ensure no root-level Exec.kdl file gets commited.

@alilleybrinker thoughts? or should the "config" folder be un-deprecated as a concept and use the resolution of the config dir to determine the expected location of the Exec.kdl file?

@j-lanson
Copy link
Collaborator

j-lanson commented Dec 9, 2024

@aamohd, you'll also have to rebase off of main and address the cli.rs conflict

@aamohd aamohd force-pushed the aamohd/file-based-config branch 5 times, most recently from 0b509bd to 5ffc34c Compare December 19, 2024 16:01
@j-lanson j-lanson marked this pull request as ready for review December 19, 2024 16:13
Copy link
Collaborator

@j-lanson j-lanson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple more things, but looking good!

hipcheck/src/main.rs Outdated Show resolved Hide resolved
hipcheck/src/cli.rs Outdated Show resolved Hide resolved
hipcheck/src/main.rs Outdated Show resolved Hide resolved
@aamohd aamohd mentioned this pull request Dec 30, 2024
Copy link
Collaborator

@j-lanson j-lanson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't forget to squash your commits into one to pass the CI check

hipcheck/src/exec.rs Outdated Show resolved Hide resolved
hipcheck/src/exec.rs Outdated Show resolved Hide resolved
hipcheck/src/session/mod.rs Outdated Show resolved Hide resolved
hipcheck/src/exec.rs Outdated Show resolved Hide resolved
hipcheck/src/session/mod.rs Outdated Show resolved Hide resolved
hipcheck/src/session/mod.rs Outdated Show resolved Hide resolved
hipcheck/src/exec.rs Outdated Show resolved Hide resolved
hipcheck/src/main.rs Outdated Show resolved Hide resolved
hipcheck/src/main.rs Outdated Show resolved Hide resolved
@aamohd aamohd force-pushed the aamohd/file-based-config branch from e6d212d to 2aaf372 Compare January 3, 2025 20:06
hipcheck/src/exec.rs Outdated Show resolved Hide resolved
hipcheck/src/exec.rs Outdated Show resolved Hide resolved
hipcheck/src/exec.rs Outdated Show resolved Hide resolved
hipcheck/src/session/mod.rs Outdated Show resolved Hide resolved
hipcheck/src/exec.rs Outdated Show resolved Hide resolved
hipcheck/src/main.rs Outdated Show resolved Hide resolved
@aamohd aamohd force-pushed the aamohd/file-based-config branch from 2aaf372 to b4e1b89 Compare January 4, 2025 13:20
Copy link
Collaborator

@j-lanson j-lanson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please carefully check for any remaining erroneous uses of unwrap().

hipcheck/src/main.rs Outdated Show resolved Hide resolved
@aamohd aamohd requested a review from j-lanson January 6, 2025 18:18
config/Config.kdl Outdated Show resolved Hide resolved
Copy link
Collaborator

@j-lanson j-lanson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One additional change (file rename), and previous comment has not been addressed yet.

@aamohd aamohd force-pushed the aamohd/file-based-config branch 2 times, most recently from d732b89 to f571a46 Compare January 6, 2025 18:50
Copy link
Collaborator

@j-lanson j-lanson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Squash down to one commit and we're probably good to go

@aamohd aamohd force-pushed the aamohd/file-based-config branch from f571a46 to 1551b88 Compare January 6, 2025 21:10
Copy link
Collaborator

@j-lanson j-lanson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please rebase off of main? Also, Rust 1.84 released today and our CI automatically uses it so you might see new clippy warnings in the CI that cause a test failure. You will need to fix those locally if they appear.

@aamohd aamohd force-pushed the aamohd/file-based-config branch from 1551b88 to 1208fa4 Compare January 10, 2025 20:52
@aamohd aamohd force-pushed the aamohd/file-based-config branch from 1208fa4 to 306a51b Compare January 10, 2025 21:09
@aamohd aamohd requested a review from j-lanson January 10, 2025 21:13
@j-lanson j-lanson merged commit ac4030c into main Jan 10, 2025
11 checks passed
@alilleybrinker alilleybrinker deleted the aamohd/file-based-config branch January 17, 2025 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product: hc Relates to the core "hc" binary type: enhancement New feature or request type: ui UI-related changes that should get heightened review.
Projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants