Container warns if permission is denied

[adamrtalbot]

Replace container failed with a warning if error code indicates the container is unauthorised rather than doesn't exist etc. We could extend this in future for relevant codes.

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]

Codecov Report

Merging #2270 (f067e87) into dev (14a14d8) will not change coverage.
The diff coverage is 66.66%.

@@           Coverage Diff           @@
##              dev    #2270   +/-   ##
=======================================
  Coverage   73.11%   73.11%           
=======================================
  Files          77       77           
  Lines        8432     8432           
=======================================
  Hits         6165     6165           
  Misses       2267     2267           

Impacted Files	Coverage Δ
nf_core/modules/lint/main_nf.py	68.09% <66.66%> (ø)

[mashehu]

Why wouldn't you want it to fail? If it is not reachable it will not work, so linting should fail IMO

[adamrtalbot]

Why wouldn't you want it to fail? If it is not reachable it will not work, so linting should fail IMO

• The role of this test is to confirm the URI is valid. If permission is denied then the URI might still be valid, but you haven't configured access. If the test is to confirm it is Docker container then...
• That's not linting, that's functionality testing and will cause all sorts of problems (e.g. now I have to use it on dedicated worker machines only instead of small testing machines). We should be adding a second, more thorough test if we want to achieve that. I would say this is covered by running the pipeline.
• It's not failed, so it shouldn't raise a failure.
• As far as I can tell, there is no way to add authentication to the request in the code? If so, which type?
• This is doing a HEAD request which has no guarantee of being supported by a registry. Sure, quay and dockerhub do we know far more registries are being used.
• 200 is far too restrictive, 200-208 are all acceptable exit status that pass.

Currently, all these linting tests are failing because of 403: nf-core/modules#3344

[adamrtalbot]

Alternatively, rather than just a blanket ERROR we could surface the error as a warning?

Access to container ${url} denied: ${exit_status}

[mirpedrol]

LGTM, thanks :)

[mirpedrol]

We were just talking with @ewels and he had more opinions about warning vs failing, this test should fail even if it's due to permissions, but maybe he wants to add something else :)
But the other changes look good! 😄

[ewels]

I'm not sure about making auth errors a warning instead of failure. This tool is primarily for use with public nf-core pipelines and as such, all modules in nf-core/modules should not require authentication.

Granted, people outside nf-core use our tools, but typically we say that the default behaviour should be matched to what we want and users can then disabled tests or set a config option to change that behaviour.

Looking through the failing tests we now have here, all failing tests seem to be real errors, all to do with using docker.io/ubuntu:20.04

[ewels]

ok, got it. docker.io/ubuntu:20.04 needs to be changed to docker.io/library/ubuntu:20.04 and then the linting test passes.

[adamrtalbot]

ok, got it. docker.io/ubuntu:20.04 needs to be changed to docker.io/library/ubuntu:20.04 and then the linting test passes.

Interesting, library is the org for simple docker images names?

[ewels]

Interesting, library is the org for simple docker images names?

Yup! After a bit of googling I finally dug that out. Full URL is actually index.docker.io/library/ubuntu apparently. I still get a failure with the docker manifest inspect test that I was trying locally, but the test run by nf-core/tools seems to pass with the full URL 🤷🏻‍♂️

[adamrtalbot]

We should at least improve the error message - I'll update to be an error.

Perhaps having a way of changing an error to warning via config be valuable.

[mirpedrol]

When we add library to the url path we won't check this URL anymore due to https://github.com/nf-core/tools/blob/master/nf_core/modules/lint/main_nf.py#L587
could we modify this check of having only one / by line.count("/") >= 1?

[mirpedrol]

I sneaked these changes into this PR 🙂

[ewels]

Thanks @adamrtalbot 👍🏻

Yes if you would like the extra config it definitely wouldn't be a bad thing. There is precedent for how this can work in the config yaml etc. elsewhere, happy to dig it out if you'd like.

Totally agree that the error message needs improving 👍🏻 (just printing the container name is a good start!)

[adamrtalbot]

I've reset to just an error plus an fstring for better error messages.