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

2311 interpret docker.registry automatically #2318

Merged
merged 8 commits into from
Jun 15, 2023

Conversation

adamrtalbot
Copy link
Contributor

@adamrtalbot adamrtalbot commented Jun 13, 2023

Based on #2313 which is based on #2306 so this is likely to get messy.

  • feat (2311): Infer docker.registry value from nextflow.config
  • Update test

Fixes #2310

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

@codecov
Copy link

codecov bot commented Jun 14, 2023

Codecov Report

Merging #2318 (10f1807) into dev (887e83e) will increase coverage by 0.00%.
The diff coverage is 90.00%.

@@           Coverage Diff           @@
##              dev    #2318   +/-   ##
=======================================
  Coverage   72.90%   72.90%           
=======================================
  Files          78       78           
  Lines        8761     8767    +6     
=======================================
+ Hits         6387     6392    +5     
- Misses       2374     2375    +1     
Impacted Files Coverage Δ
nf_core/__main__.py 57.16% <ø> (ø)
nf_core/modules/lint/__init__.py 83.41% <87.50%> (-0.01%) ⬇️
nf_core/utils.py 81.40% <100.00%> (ø)

Copy link
Member

@mirpedrol mirpedrol left a comment

Choose a reason for hiding this comment

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

LGTM, only one small comment

nf_core/utils.py Outdated
@@ -303,6 +303,11 @@ def fetch_wf_config(wf_path, cache_config=True):
return config


def parse_nf_config(wf_path, cache_config=True):
Copy link
Member

Choose a reason for hiding this comment

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

is this function used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No I don't think it is!

@adamrtalbot
Copy link
Contributor Author

@mirpedrol There is a slight problem where it only looks for the most basic nextflow.config file. You can override the registry on the command line but do we want it to be more intelligent?

@mirpedrol
Copy link
Member

This is only for linting modules, right? If I'm not wrong, we will always provide the registry through a nextflow.config file in nf-core/modules. In that case, I think it's fine that it only checks nextflow.config. Or is there any edge case that I am missing?

@adamrtalbot
Copy link
Contributor Author

It is also used with pipelines I believe, so people may override the standard registry with that. As long as it's in the /nextflow.config (pipeline) or tests/config/nextflow.config (modules) it should work.

@adamrtalbot
Copy link
Contributor Author

Oh but fetch_wf_config() runs nextflow config -flat. So maybe it's OK?

@mirpedrol
Copy link
Member

Not sure I'm following, do you mean that fetch_wf_config() does only retrieve configuration values from nextflow.config?

@mirpedrol
Copy link
Member

Ok I think I misunderstood your first question, sorry!

Oh but fetch_wf_config() runs nextflow config -flat. So maybe it's OK?

Now I have double-checked this and I think it's ok :) nextflow config will return all the configuration values

@adamrtalbot adamrtalbot merged commit c96c1d1 into dev Jun 15, 2023
@adamrtalbot adamrtalbot deleted the 2311-interpret-docker.registry-automatically branch June 15, 2023 16:32
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