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

Default Docker registry in template is quay.io #2233

Merged
merged 10 commits into from
Apr 26, 2023

Conversation

adamrtalbot
Copy link
Contributor

@adamrtalbot adamrtalbot commented Apr 3, 2023

Sets the default container registry to quay.io. This can be modified directly using the config. For example, docker.registry = mysecret.registry.io and a process with container specified as multiqc:dev will result in mysecret.registry.io/multiqc:dev, however specifying the full URL such as quay.io/biocontainers/multiqc:latest will result in that container being used instead.

This shouldn't affect existing modules except in the case here someone is pulling an image from docker.io and has specified it using the container name only (e.g. ubuntu:20.04). This is likely to be pipeline specific and should be resolved pipeline side anyway.

Fixes #1948

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

- Adds parameter `--registry` which specifies which base registry to use
- Defaults to quay.io/biocontainers to match existing modules
- When it is changed, any containers specified by name (e.g.
  multiqc:latest) will be pulled from the registry defined in
`--registry`.
- This is aimed at using `--registry public.ecr.aws/biocontainers`
@codecov
Copy link

codecov bot commented Apr 3, 2023

Codecov Report

Merging #2233 (f018d63) into dev (9e7f50e) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##              dev    #2233   +/-   ##
=======================================
  Coverage   73.06%   73.06%           
=======================================
  Files          77       77           
  Lines        8404     8404           
=======================================
  Hits         6140     6140           
  Misses       2264     2264           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@adamrtalbot
Copy link
Contributor Author

This doesn't seem to work on cloud. Closing until we know more.

@adamrtalbot adamrtalbot closed this Apr 4, 2023
@adamrtalbot adamrtalbot reopened this Apr 4, 2023
@adamrtalbot
Copy link
Contributor Author

Turns out it should be fine on cloud, however when using Tower you must set the parameter directly in the config, e.g.:

docker.registry = "myregistry.io"

Changes:
 - Sets docker.registry directly in the Docker scope
 - Removes param.container_registry which might have caused problems
   in various environments.
@adamrtalbot adamrtalbot changed the title Default Docker registry configurable on the command line [WIP] Default Docker registry configurable on the command line Apr 5, 2023
@mirpedrol mirpedrol added this to the 2.8 milestone Apr 25, 2023
@adamrtalbot adamrtalbot changed the title [WIP] Default Docker registry configurable on the command line Default Docker registry configurable on the command line Apr 25, 2023
@adamrtalbot
Copy link
Contributor Author

@mirpedrol what do you think of this? It's kinda dumb and I'm not 100% but it's the best idea I've had so far.

@adamrtalbot adamrtalbot changed the title Default Docker registry configurable on the command line Default Docker registry in template is quay.io Apr 25, 2023
@mirpedrol
Copy link
Member

Looks good, not sure if there are some weird cases where it will fail, but I think it will work for now

@mirpedrol
Copy link
Member

Will need to merge this one for tests in #2249 to pass. Are you happy with merging? I was wondering if we should expose the parameter registry as you did in your POC?

@adamrtalbot
Copy link
Contributor Author

I chatted with @drpatelh about this, we think if you're advanced enough to use a custom registry you should set it via config.

@adamrtalbot
Copy link
Contributor Author

Will need to merge this one for tests in #2249 to pass. Are you happy with merging? I was wondering if we should expose the parameter registry as you did in your POC?

I'll rebase that branch now, but yes I think this one is good to go. We need to have a notice alerting pipeline developers to the change. Before we merge, is there anywhere good I can add this or shall we stick with documentation and Slack?

@mirpedrol
Copy link
Member

Slack will be the best place to inform about the change 🙂

@adamrtalbot
Copy link
Contributor Author

adamrtalbot commented Apr 26, 2023

I've applied the template change to RNASeq to give it a thorough test, will merge if it works (looking good so far).

@adamrtalbot adamrtalbot merged commit 271b84c into nf-core:dev Apr 26, 2023
@adamrtalbot adamrtalbot deleted the configurable_docker_registry branch April 26, 2023 16:58
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.

2 participants