Switch out the API #37
Conversation
MillironX
requested review from
ewels,
mashehu,
MatthiasZepper,
adamrtalbot and
edmundmiller
|
Things that I want to see before merging this PR:
I have personally stress-tested this PR on taxprofiler, since that pipeline is near and dear to me. I want someone else (who doesn't have a conflict of interest) to do their best to break it on some of our most problematic repos, though. Hopefully we can get this version into the template before the hackathon! |
|
For modules, I just kicked off this one again: https://github.com/nf-core/modules/pull/4726/checks, might be a good candidate |
That one is still pointed at |
|
Looking good for now: https://github.com/nf-core/modules/actions/runs/7845940111/job/21411506756 |
|
All right, I'm giving @nf-core/sarek team one week before I merge and tag this. Otherwise I won't have time to get the pipeline template updated before the hackathon. Have fun! |
There was a problem hiding this comment.
Excellent work! Thanks a lot!
Minor remark: When I ran npm audit (v21.6.1/npm v10.2.4), it warned me about several security vulnerabilities in the dependencies, including a highly critical one in the JSON parsing. Although we trust the API in that case, I think it can't harm to run an npm audit fix and commit the updated package-lock.json.
Per @ewels request, the API on nf-co.re includes a "published_at" field with the date of each release. For future compatibility without breaking anything current, add an optional field that can handle this additional information.
Basically, a stand-in for OctoKit, but smarter.
Although it seems like we're moving backwards, the new API only supports a single data dump. I still like the iterative nature rather, so tack on array support.
This functionality is now offloaded to the API.
Not needed due to the new API
…rsion This is to preserve consistency with the new API
…adURL Another API consistency change.
…nloadUrlAll Another API consistency update.
This is no longer needed under the new API.
The "install_nextflow" tests generate a new install of Nextflow every time they are run based on the "RUNNER_TEMP" environment variable. Deal with this by adding our RUNNER_TEMP variable to the ava config and making sure none of these installs get committed.
Well, here it is: the big rewrite. I don't want this merged yet, but want it to be available for everyone to be testing against. Please try some of your biggest and baddest testing workflows against this branch (I've committed the sin of committing the build to the branch so we can do that).
This is a breaking change, so I want to properly deprecate the unused parameters on v1 today, then move this to v2 once it's properly tested.