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

fix: Drop container error to warning #2396

Merged

Conversation

adamrtalbot
Copy link
Contributor

@adamrtalbot adamrtalbot commented Aug 14, 2023

Changes:

  • Container registries do not have a consistent API, so calling them
    for container presence doesn't seem to work on GHCR or NVCR.
  • In addition some registries require authentication and do not have a consistent method of authentication, likely oauth2 but not clear.
  • This drops the error to a warning so we don't fail tests.

Relates to nf-core/modules#3693 and #2393

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 Aug 14, 2023

Codecov Report

Merging #2396 (c570b1d) into dev (9d63f93) will increase coverage by 0.02%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##              dev    #2396      +/-   ##
==========================================
+ Coverage   71.12%   71.14%   +0.02%     
==========================================
  Files          87       87              
  Lines        9408     9410       +2     
==========================================
+ Hits         6691     6695       +4     
+ Misses       2717     2715       -2     
Files Changed Coverage Δ
nf_core/modules/lint/main_nf.py 68.38% <50.00%> (+0.10%) ⬆️

... and 4 files with indirect coverage changes

Changes:
 - Container registries do not have a consistent API, so calling them
   for container presence doesn't seem to work on GHCR or NVCR.
 - This drops the error to a warning so we don't fail tests.

Relates to nf-core/modules#3693 and nf-core#2393
@adamrtalbot adamrtalbot force-pushed the drop_docker_container_error_to_warn branch from c224ba4 to 67a84e6 Compare August 15, 2023 10:17
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

@adamrtalbot
Copy link
Contributor Author

You guys think this is OK? I wasn't sure if it was a good idea, but then again I don't see how to solve the container problem.

@mashehu
Copy link
Contributor

mashehu commented Aug 15, 2023

yeah, it's a bit of cheating, but if it will not be too much to the point that we just start ignoring these warnings, it is fine.

@adamrtalbot adamrtalbot merged commit e964a40 into nf-core:dev Aug 15, 2023
20 checks passed
@adamrtalbot
Copy link
Contributor Author

Okey dokey, then let's goooo.

@adamrtalbot adamrtalbot deleted the drop_docker_container_error_to_warn branch August 15, 2023 15:16
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