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

enable setting defaulBranch for pipeline repo creation #1959

Merged
merged 13 commits into from
Oct 20, 2022

Conversation

fabianegli
Copy link
Contributor

@fabianegli fabianegli commented Oct 19, 2022

For reference: https://nfcore.slack.com/archives/CE5LG7WMB/p1665850093986309

When a local .gitconfig specified the init.defaultBranch to anything else than master one sync test would fail.

To make the tests system config agnostic, it is necessary to have a different way of telling nf-core tools what it should use as a default branch when creating new pipeline repositories. This PR implements just that in the internals and uses the feature to make tests pass on systems with init.defaultBranch not set to master. It does not expose this feature to the CLI - this should maybe be the topic of another PR that then also updates the docs.

PR checklist

  • This comment contains a description of changes (with reason)
  • CHANGELOG.md is updated
  • If you've fixed a bug or added code that should be tested, add tests!
  • Documentation in docs is updated

@codecov
Copy link

codecov bot commented Oct 19, 2022

Codecov Report

Merging #1959 (b4789a6) into dev (6cd9134) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##              dev    #1959   +/-   ##
=======================================
  Coverage   61.10%   61.10%           
=======================================
  Files          37       37           
  Lines        4677     4677           
=======================================
  Hits         2858     2858           
  Misses       1819     1819           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@fabianegli
Copy link
Contributor Author

I wonder why the pipeline creation CI fails 🤷

Copy link
Contributor

@Aratz Aratz left a comment

Choose a reason for hiding this comment

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

Great work! Suggested a couple of changes.

Additionally, I think it would be great if you could edit test_create.py to check the default branch is set correctly

def test_pipeline_creation(self):
assert self.pipeline.template_params["name"] == self.pipeline_name
assert self.pipeline.template_params["description"] == self.pipeline_description
assert self.pipeline.template_params["author"] == self.pipeline_author
assert self.pipeline.template_params["version"] == self.pipeline_version

nf_core/create.py Outdated Show resolved Hide resolved
nf_core/create.py Outdated Show resolved Hide resolved
nf_core/create.py Outdated Show resolved Hide resolved
@fabianegli
Copy link
Contributor Author

Ready for re-review.

@fabianegli fabianegli requested a review from Aratz October 20, 2022 08:17
@Aratz
Copy link
Contributor

Aratz commented Oct 20, 2022

Looks great! You did not address adapting the tests in test_create.py though

@fabianegli
Copy link
Contributor Author

fabianegli commented Oct 20, 2022

Thank you for the flowers.

Forgot the note about the test. Added one.

@fabianegli
Copy link
Contributor Author

There's maybe still an issue with the default branch not being included in the template. I am not familiar enough with the templates and would like some input from someone who is about wether the default branch should be part of the template or not.

nf_core/create.py Show resolved Hide resolved
nf_core/create.py Show resolved Hide resolved
@fabianegli fabianegli merged commit 5b8ed27 into nf-core:dev Oct 20, 2022
@fabianegli fabianegli deleted the initial-git-branch-setting branch October 20, 2022 18:00
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