-
Notifications
You must be signed in to change notification settings - Fork 196
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
Biocontainer api #1110
Biocontainer api #1110
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #1110 +/- ##
==========================================
+ Coverage 72.31% 72.38% +0.07%
==========================================
Files 36 36
Lines 4547 4552 +5
==========================================
+ Hits 3288 3295 +7
+ Misses 1259 1257 -2
Continue to review full report at Codecov.
|
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.
Haven't tested it, but general code read-over looks good!
Co-authored-by: Phil Ewels <phil.ewels@scilifelab.se>
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.
Looks good! Just found a few minor things.
continue | ||
else: | ||
singularity_image = img | ||
return docker_image["image_name"], singularity_image["image_name"] |
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.
Are we guaranteed by the API to at least get both one Docker and Singularity image tag? Since this might raise an error otherwise.
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 think if there the response is successful, then in theory, there should be at least one of both. And if one of them stays None
then that's actually okay for the template. But yeah might be worth trying this out a little bit and make it a bit robuster ....
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.
Yes it should be fine, it either raises an error, returns the images or None
, and None
can be handled by nf-core modules create
(will just print YOUR-TOOL-HERE
in the main.nf
file instead)
Co-authored-by: Erik Danielsson <53212377+ErikDanielsson@users.noreply.github.com>
Co-authored-by: Phil Ewels <phil.ewels@scilifelab.se>
Use the Biocontainers API instead of quay.io to get container tags for Docker and Singularity containers as suggested in #875
I have not implemented any questionary selection now, as you have suggested @ewels . Mainly because I think it would be good to have the most recent version as default, and allowing users to choose a container tag/version for both Docker and Singularity could mean they choose different builds. The linter should complain about this, or we could directly check within
nf-core modules create
, but easiest would be to just avoid this in the first place.Wouldn't be too hard to add it in though so up for discussion I'd say :-)
PR checklist
CHANGELOG.md
is updateddocs
is updated