-
Notifications
You must be signed in to change notification settings - Fork 190
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
base: dev
Are you sure you want to change the base?
Conversation
@nf-core-bot fix linting |
74bacb8
to
937e442
Compare
@nf-core-bot fix linting |
# 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"]) |
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 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.
On hold until further notice. Nearly complete in case anyone wants to pick it up. Suggestion is to guess the URL for the supplied commit (if it looks like a hash - is the right length and characters) and try it, see if we get a 404 or not. That way we don't need to fetch the commit history for the repo. |
This PR has been tagged as awaiting-changes or awaiting-feedback by an nf-core contributor. Remove stale label or add a comment if it is still useful. |
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
CHANGELOG.md
is updateddocs
is updated