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

Added possibility to call amend/try-amend multiple times from easystack #4667

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

Crivella
Copy link

@Crivella Crivella commented Oct 4, 2024

Related to:

This 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:

  • Any currently existing easystack that makes use of a pattern like
      options:
        - try-amend:
          - opt=1
          - 2
          - 3
    
    in order to set --try-amend=opt=1,2,3 would not work anymore

easybuild/tools/options.py Outdated Show resolved Hide resolved
ocaisa
ocaisa previously approved these changes Oct 4, 2024
Copy link
Member

@ocaisa ocaisa left a 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))

@cgross95
Copy link

cgross95 commented Oct 4, 2024

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):

@boegel boegel added enhancement easystack Issues and PRs related to easystack files labels Oct 4, 2024
easybuild/tools/options.py Outdated Show resolved Hide resolved
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)
Copy link
Member

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?

Copy link
Author

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

'from-pr': [1234, 2345],

Copy link
Author

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

@boegel
Copy link
Member

boegel commented Oct 4, 2024

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))

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]>
@bartoldeman
Copy link
Contributor

@boegel does this still need work or can it be merged now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
easystack Issues and PRs related to easystack files enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants