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

Linting error if quay.io found in biocontainer name #2278

Merged
merged 8 commits into from
May 9, 2023

Conversation

adamrtalbot
Copy link
Contributor

Changes:

  • Linting error if container starts with quay.io/biocontainers but not quay.io/biocontainers/mulled

Fixes #2276

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

Changes:
 - Linting error if container starts with quay.io/biocontainers but not
   quay.io/biocontainers/mulled

Fixes nf-core#2276
Copy link
Member

@ewels ewels left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏻

For a separate issue / PR - this file really needs breaking into many smaller ones with different tests. Also the docs are severely lacking.. 😞

nf_core/modules/lint/main_nf.py Outdated Show resolved Hide resolved
Copy link
Member

@ewels ewels left a comment

Choose a reason for hiding this comment

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

Wait, need to add a self.passed when the test is passing..

@adamrtalbot
Copy link
Contributor Author

adamrtalbot commented May 5, 2023

Wait, need to add a self.passed when the test is passing..

Do we want one for the general container? E.g. if docker image name looks valid ✅

Puts container status check into a single if statement
@adamrtalbot
Copy link
Contributor Author

Wait, need to add a self.passed when the test is passing..

Do we want one for the general container? E.g. if docker image name looks valid ✅

Nevermind, I've copied the existing format. Also updated one of the checks to occur once.

@adamrtalbot
Copy link
Contributor Author

@drpatelh This PR now changes if you are removing quay.io/ from mulled containers as well. Shall we drop it?

@adamrtalbot adamrtalbot requested a review from ewels May 9, 2023 13:51
@codecov
Copy link

codecov bot commented May 9, 2023

Codecov Report

Merging #2278 (00bd8ef) into dev (12404d2) will increase coverage by 0.07%.
The diff coverage is 57.14%.

@@            Coverage Diff             @@
##              dev    #2278      +/-   ##
==========================================
+ Coverage   73.05%   73.12%   +0.07%     
==========================================
  Files          77       77              
  Lines        8424     8433       +9     
==========================================
+ Hits         6154     6167      +13     
+ Misses       2270     2266       -4     
Impacted Files Coverage Δ
nf_core/modules/lint/main_nf.py 68.19% <57.14%> (+0.10%) ⬆️

... and 9 files with indirect coverage changes

@adamrtalbot adamrtalbot merged commit 14b0736 into nf-core:dev May 9, 2023
@adamrtalbot adamrtalbot deleted the catch_modules_with_quay.io branch May 9, 2023 14:51
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.

4 participants