Skip to content

Allow use of --copy-ec with --from-pr #3473

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.

Sign up for GitHub

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

Jump to bottom
Merged
ocaisa merged 29 commits into from
Nov 30, 2020
Merged

Allow use of --copy-ec with --from-pr #3473

ocaisa merged 29 commits into from
Nov 30, 2020

Conversation

@ocaisa
Copy link
Member

@ocaisa ocaisa commented Oct 19, 2020 •
edited
Loading

edit: fixes #3178

@boegel boegel added the bug fix label Oct 19, 2020
@boegel boegel added this to the next release (4.3.1) milestone Oct 19, 2020
@boegel
Copy link
Member

boegel commented Oct 19, 2020

@ocaisa Are you up for enhancing the test for --copy-ec to cover the use case (combo with --from-pr) you're fixing here?

@ocaisa
Copy link
Member Author

ocaisa commented Oct 19, 2020

I'm working on it now

@easybuilders easybuilders deleted a comment from boegelbot Oct 19, 2020
@ocaisa
Copy link
Member Author

ocaisa commented Oct 19, 2020

This falls short, it won't also copy over the required patch files (having said that, the option --copy-ec never promised that)

@ocaisa
Copy link
Member Author

ocaisa commented Oct 19, 2020

@boegel Ready for review

@hound hound bot deleted a comment from boegel Oct 19, 2020
ocaisa
ocaisa commented Oct 19, 2020
View reviewed changes
boegel
boegel requested changes Oct 19, 2020
View reviewed changes
@easybuilders easybuilders deleted a comment from boegelbot Oct 19, 2020
@ocaisa
Copy link
Member Author

ocaisa commented Oct 20, 2020

I think I've made this overly complicated, now that I've had a look at existing GitHub functionality I could have just used fetch_files_from_pr() and this would be done

@ocaisa ocaisa requested a review from boegel October 20, 2020 14:24
@boegel boegel mentioned this pull request Oct 25, 2020
Merged
4 tasks
boegel
boegel requested changes Oct 25, 2020
View reviewed changes
Copy link
Member

@boegel boegel left a comment

Choose a reason for hiding this comment

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

I spent quite a bit of time cleaning things up on top of these changes to clean things up a bit, see #3482

easybuild/main.py
copy_ec = options.copy_ec and not tweaked_ecs_paths
if copy_ec or options.fix_deprecated_easyconfigs or options.show_ec:
if options.from_pr:
# pull in the paths to all the changed files in the PR (need to do this in a new temp dir)
Copy link
Member

@boegel boegel Oct 20, 2020

Choose a reason for hiding this comment

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

@ocaisa This whole code block below if options.from_pr should be moved to a separate function imho, blowing up the logic in main like this doesn't help with keeping things manageable...

Not sure what an appropriate function name would be, but since it's specific to --from-pr I would park it in easybuild.tools.github?

@boegel boegel modified the milestones: 4.3.1 (next release), 4.3.2 Oct 25, 2020
@ocaisa ocaisa merged commit 6e5be8d into easybuilders:develop Nov 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@boegel boegel boegel requested changes

Assignees

No one assigned

Labels

bug fix

Projects

None yet

Milestone

4.3.2

Development

Successfully merging this pull request may close these issues.

eb --copy-ec does not work with --from-pr yet

2 participants

@ocaisa @boegel