Skip to content

Download pipelines with authenticated GH API calls#3607

Merged
JulianFlesch merged 27 commits into
devfrom
jpfeuffer-patch-1
Nov 6, 2025
Merged

Download pipelines with authenticated GH API calls#3607
JulianFlesch merged 27 commits into
devfrom
jpfeuffer-patch-1

Conversation

@jpfeuffer

@jpfeuffer jpfeuffer commented Jun 5, 2025

Copy link
Copy Markdown
Contributor

For repos that follow the nf-core template but need authentication.

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

@jpfeuffer jpfeuffer requested a review from fabianegli June 5, 2025 15:20
@github-actions

This comment was marked as outdated.

Sign in to view
@jpfeuffer jpfeuffer changed the base branch from main to dev June 5, 2025 15:21
@jpfeuffer

jpfeuffer commented Jun 5, 2025

Copy link
Copy Markdown
Contributor Author

Oops wrong base branch. Fixed.

@jpfeuffer

jpfeuffer commented Jun 6, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Done. Test failures (most likely) unrelated or at least I have no idea what they mean.

@jpfeuffer jpfeuffer requested a review from mashehu June 6, 2025 09:29
@MatthiasZepper

MatthiasZepper commented Jul 30, 2025

Copy link
Copy Markdown
Member

Done. Test failures (most likely) unrelated or at least I have no idea what they mean.

There is a problem mapping your CLI arguments to the parameters of the DownloadWorkflow class init() function. It seems they are disordered, and therefore you get the TypeError("object of type 'bool' has no len()")

Apart from that, please mind that there is a major refactor of Downloads ongoing (#3634). Preferably, all new contributions would already be based on and point to this new structure.

This was referenced Aug 22, 2025
Closed
Merged
@JulianFlesch

JulianFlesch commented Aug 29, 2025

Copy link
Copy Markdown
Contributor

How does this fit with the pipeline downloads refactoring @MatthiasZepper @jpfeuffer?
Is this salvageable or maybe already covered by the PRs that were merged?

@jpfeuffer

jpfeuffer commented Aug 29, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Good question. Who knows more about the refactor?
From what I gathered from @MatthiasZepper s comment this might not yet be covered.
This is mainly about the repo download, not the download of the individual containers etc.
(Which from what I could see was the main focus of the refactor but judging from its size probably not the only focus)

@jpfeuffer

jpfeuffer commented Aug 29, 2025

Copy link
Copy Markdown
Contributor Author

Yes, I checked. And the mechanism for a download of the repo data/files is still the same.
The patch can be carried over as-is, just for the new folder structure.

@jpfeuffer

jpfeuffer commented Aug 29, 2025

Copy link
Copy Markdown
Contributor Author

And well there needs to be a little fix for the CLI apparently. Note that I just added the CLI option because I still wanted to allow downloads from the Zip URLs because they will never be rate-limited. While non-authenticated downloads from the API can be.

@JulianFlesch JulianFlesch self-assigned this Sep 4, 2025
@codecov

codecov Bot commented Sep 18, 2025 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 77.37%. Comparing base (659c624) to head (e54481b).
⚠️ Report is 28 commits behind head on dev.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jpfeuffer

jpfeuffer commented Sep 18, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I quickly let copilot rebase the changes. (and fixed the cli bug)
If you want more code coverage you would need to think about a test case with a Git token. I am not familiar enough with your internal CI to suggest a way forward here.
@JulianFlesch

@JulianFlesch JulianFlesch added this to the 3.5.0 milestone Oct 9, 2025
@jpfeuffer
Address PR review comments: refactor download functionality
1d49215 
- Rename --api-download to --authenticated for better UX
- Replace os.rename with Pathlib operations
- Refactor download_wf_files method to reduce code duplication
- Rename compress to compress_type consistently across codebase
- Update all references and tests accordingly
@jpfeuffer
remove duplication
09a8fb0
@jpfeuffer
better code flow.
7310e75
@jpfeuffer
fmt
9b8d397
JulianFlesch
JulianFlesch requested changes Oct 13, 2025
View reviewed changes

@JulianFlesch JulianFlesch left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry for taking my time with this review. The repo download could probably be simplified further and I think we should get away without adding a new flag, if I am not missing something.

Is it also a target of this PR to add downloads from different sources (i.e. private repos)?

Comment thread nf_core/pipelines/download/download.py Outdated
Comment thread nf_core/__main__.py Outdated
@jpfeuffer

jpfeuffer commented Oct 13, 2025

Copy link
Copy Markdown
Contributor Author

Yep, private/internal (GitHub) repos should be supported by this. This is basically my use case.

The download_url unification should be addressed by my latest changes.

JulianFlesch
JulianFlesch requested changes Oct 13, 2025
View reviewed changes
Comment thread nf_core/pipelines/download/download.py
Comment thread nf_core/pipelines/download/download.py Outdated
JulianFlesch
JulianFlesch reviewed Oct 14, 2025
View reviewed changes
Comment thread nf_core/pipelines/download/download.py Outdated
@JulianFlesch

JulianFlesch commented Oct 14, 2025

Copy link
Copy Markdown
Contributor

Ok, my revised suggestion:

  • Read the authenticated status from gh_api in favor of adding a new flag that uses gh_api anyway (this comment)
  • Keep the two wf_download_url to not cause new issues in workflows with quotas
  • Keep only one of the two redundant functions as suggested in this comment

What do you think?

Thanks for this feature @jpfeuffer and sorry for being a bit picky on these details, I am just trying to keep the complexity as low as can be.

@JulianFlesch

JulianFlesch commented Nov 6, 2025

Copy link
Copy Markdown
Contributor

Hi @jpfeuffer , I will quickly commit my most recent comments and merge this PR. This is to avoid your feature becoming stale and to have this available in the dev version.
Thanks for getting it this far 👍

@jpfeuffer

jpfeuffer commented Nov 6, 2025

Copy link
Copy Markdown
Contributor Author

Thank you, yes the comments make sense I think. I have just been busy with other stuff.

@JulianFlesch JulianFlesch merged commit 8ff5da6 into dev Nov 6, 2025
116 checks passed
@JulianFlesch JulianFlesch deleted the jpfeuffer-patch-1 branch November 6, 2025 16:07
@github-project-automation github-project-automation Bot moved this from In Progress to Done in nf-core infrastructure projects Nov 6, 2025
@JulianFlesch JulianFlesch mentioned this pull request Nov 6, 2025
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MatthiasZepper MatthiasZepper MatthiasZepper left review comments

@JulianFlesch JulianFlesch JulianFlesch approved these changes

+1 more reviewer

@rrahn rrahn rrahn left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@JulianFlesch JulianFlesch

Labels

None yet

Projects

nf-core infrastructure projects
Status: Done

Milestone

3.5.0

Development

Successfully merging this pull request may close these issues.

5 participants

@jpfeuffer @MatthiasZepper @JulianFlesch @rrahn @mashehu