-
Notifications
You must be signed in to change notification settings - Fork 203
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
allow usingamend/try-amend
multiple times in an easystack file entry
#4667
allow usingamend/try-amend
multiple times in an easystack file entry
#4667
Conversation
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.
LGTM but maybe @boegel wants to take a look as there is a (very unlikely to be used) breaking change in here (as mentioned in #4667 (comment))
Thank you for working on this! I just tested and can verify that it works exactly as desired for our use case. If this does get merged in as is, I wrote up a documentation PR (that I am happy to edit as desired): |
args.append(option) | ||
elif value is False: | ||
args.append('--disable-' + option[2:]) | ||
elif value is not None: | ||
if isinstance(value, (list, tuple)): | ||
value = ','.join(str(x) for x in value) |
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.
Can we also cover this with an enhanced test?
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 that is already covered with
easybuild-framework/test/framework/options.py
Line 7239 in 3d29b0a
'from-pr': [1234, 2345], |
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.
But i can add a separate test if needed
It's OK I think, especially since easystack files are (still) an experimental feature currently... That said, it's high time we remove the experimental tag for easystack files, definitely for EasyBuild 5.0... |
Co-authored-by: Kenneth Hoste <[email protected]>
@boegel does this still need work or can it be merged now? |
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.
lgtm
amend/try-amend
multiple times from easystackamend/try-amend
multiple times in an easystack file entry
Related to:
--try-amend
statements in Easystack entry #4643This change checks if the option in the easystack belongs to a list of options that allows to be defined multiple times (could be defined in a more generic location) and instead of setting it's value as a list, will set it multiple times to the cli arguments.
Possible breaking change:
--try-amend=opt=1,2,3
would not work anymore