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

feat: use default config path when no arg for -f is provided #2939

Merged
merged 2 commits into from
Jan 28, 2025

Conversation

waveywaves
Copy link
Contributor

part of #1706

@waveywaves waveywaves force-pushed the feat/default-config-name branch 3 times, most recently from 474f740 to d4df7bc Compare January 27, 2025 16:25
@waveywaves waveywaves marked this pull request as ready for review January 27, 2025 16:27
@waveywaves waveywaves marked this pull request as draft January 27, 2025 16:27
@waveywaves waveywaves force-pushed the feat/default-config-name branch 2 times, most recently from fd35229 to 9837047 Compare January 27, 2025 16:31
@waveywaves waveywaves marked this pull request as ready for review January 27, 2025 16:32
@waveywaves waveywaves force-pushed the feat/default-config-name branch 5 times, most recently from f03e9e8 to 6a01faa Compare January 27, 2025 17:57
@waveywaves waveywaves force-pushed the feat/default-config-name branch from 6a01faa to 641f525 Compare January 27, 2025 17:58
Copy link
Contributor

@Razz4780 Razz4780 left a comment

Choose a reason for hiding this comment

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

Looks like these no longer have to be Options, no? 👀

@waveywaves
Copy link
Contributor Author

Looks like these no longer have to be Options, no? 👀

@Razz4780 think it does because PathBuf can either be Some (if -f is provided with or without a value) and None (-f is not provided at all). When we use default_value instead of default_missing_value then we wouldn't need Option because there would always be Some value which is the default value we pass to default_value. default_missing_value checks if -f has a value. If it does not then it uses ./mirrord.json as the argument to the -f flag.

Let me know if I am making sense.

@Razz4780
Copy link
Contributor

Looks like these no longer have to be Options, no? 👀

@Razz4780 think it does because PathBuf can either be Some (if -f is provided with or without a value) and None (-f is not provided at all). When we use default_value instead of default_missing_value then we wouldn't need Option because there would always be Some value which is the default value we pass to default_value. default_missing_value checks if -f has a value. If it does not then it uses ./mirrord.json as the argument to the -f flag.

Let me know if I am making sense.

Ah yes, this is correct. Sorry for confusion

@Razz4780 Razz4780 added this pull request to the merge queue Jan 28, 2025
Copy link
Member

@t4lz t4lz left a comment

Choose a reason for hiding this comment

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

Two small comments on the PR:

  1. The IDE plugins use a default path of .mirrord/mirrord.json, I think the default path in the CLI should be the same.
  2. Currently users are able to specify a config file without a = sign, like this: -f my-mirrord-config.json. After this change, will that still work? If not, this is a breaking change.

@Razz4780 Razz4780 removed this pull request from the merge queue due to a manual request Jan 28, 2025
@waveywaves
Copy link
Contributor Author

waveywaves commented Jan 28, 2025

@t4lz

  1. I will update the default path to be the same then.

  2. I tested this PR with -f my-mirrord-config.json like config with the following command

./target/universal-apple-darwin/debug/mirrord exec -f ./my-mirrord-mirrord.json -t deployment/kotlin-guestbook bash

and it works, don't believe this will break the existing user experience. I will add a test just in case

@Razz4780 Razz4780 enabled auto-merge January 28, 2025 11:11
@Razz4780 Razz4780 disabled auto-merge January 28, 2025 11:11
@waveywaves
Copy link
Contributor Author

@t4lz @Razz4780 PR has been updated 🫡

@Razz4780 Razz4780 added this pull request to the merge queue Jan 28, 2025
Merged via the queue into metalbear-co:main with commit 9684324 Jan 28, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants