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

Allow nf-core download -r to download commits #2906

Draft
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

fellen31
Copy link

@fellen31 fellen31 commented Apr 4, 2024

Mimic the behaviour of nextflow run -r and allow selected commit(s) to be downloaded.

https://nfcore.slack.com/archives/CE5LG7WMB/p1712143905189639

Not sure about the effect it will have on Tower downloads.

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

@mashehu
Copy link
Contributor

mashehu commented Apr 4, 2024

@nf-core-bot fix linting

@fellen31
Copy link
Author

fellen31 commented Apr 4, 2024

@nf-core-bot fix linting

Comment on lines +1013 to +1016
# Get commit information from github api
commit_response = gh_api.safe_get(f"https://api.github.com/repos/{pipeline}/commits?sha={branch['name']}")
for commit in commit_response.json():
wf_commits.append(commit["sha"])
Copy link
Member

Choose a reason for hiding this comment

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

I am a bit scared of this - is it really getting every single commit from a repo? In some repos that could be a huge response.

I wonder if we move this into a separate function call and only do it if we need to - that is, once we're sure that the user is passing a commit and is not going to use a branch or a tag?

I suspect that the number of people wanting exact commits to be a tiny proportion, so I don't want to slow down / risk crashes for the majority.

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.

None yet

4 participants