-
Notifications
You must be signed in to change notification settings - Fork 359
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
Implement --md5 and --channel-subdir for non-explicit env export #2672
Conversation
I've just merged the fix for the |
Done, I've also added |
|
if (!no_build) | ||
{ | ||
dependencies << "=" << v.build_string; | ||
} | ||
if (no_md5 == -1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In which scenario no_md5
would be equal to -1
if it's configured as a flag and initialized to 0
?
Is there any test for this use case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think when you pass --md5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--md5
is just a negation for no_md5
and they are both flags, so I don't think it's ever going to be equal to -1
. We could either use add_option
instead of add_flag
to allow the user to set it to -1
or other int
and have that use case, or leave it as a bool
and not have that use case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also a bit confused why no_md5
is not a boolean. If there are more complex cases than boolean, I think an enum would be more readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just experimentally verified the following behavior:
- Passing nothing:
no_md5=0
- Passing
--no-md5
:no_md5=1
- Passing
--md5
:no_md5=-1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm merging this for now but I'm happy to improve on it using an enum or something else in a follow up PR
This keeps the old behaviour of defaulting to md5 output for explicit files and no md5 output for environment files, but allows for outputting md5 with
--md5
for environment files.