cli icon indicating copy to clipboard operation
cli copied to clipboard

cli/trust: TagTrusted: lazily request APIClient

Open thaJeztah opened this issue 8 months ago • 5 comments

Seeing various test-failures that could be a regression, related to

  • https://github.com/docker/cli/pull/5894

cli/trust: TagTrusted: remove, and inline code

Commit e37d814ce96b01393a400c081666ea1cca2eb8bd moved the image.TagTrusted function to the trust package, but changed the signature slightly to accept an API client, instead of requiring the command.Cli. However, this could result in situations where the Client obtained from the CLI was not correctly initialized, resulting in failures in our e2e test;

=== FAIL: e2e/global TestPromptExitCode/plugin_upgrade (9.14s)
    cli_test.go:203: assertion failed: 
        Command:  docker plugin push registry:5000/plugin-content-trust-upgrade:next
        ExitCode: 1
        Error:    exit status 1
        Stdout:   The push refers to repository [registry:5000/plugin-content-trust-upgrade]
        24ec5b45d59b: Preparing
        6a594992d358: Preparing
        224414d1b129: Preparing
        24ec5b45d59b: Preparing
        6a594992d358: Preparing
        224414d1b129: Preparing
        
        Stderr:   error pushing plugin: failed to do request: Head "https://registry:5000/v2/plugin-content-trust-upgrade/blobs/sha256:6a594992d358facbbc4ab134bbbba77cb91e0adee6ff0d6103403ff94a9b796c": http: server gave HTTP response to HTTPS client
        
        
        Failures:
        ExitCode was 1 expected 0
        Expected no error

This patch changes the signature to accept an "APIClientProvider" so that the Client is obtained the moment when used.

We should look what exactly causes this situation, and if we can make sure that requesting the Client() will always produce the client with the expected configuration.

While looking at the code, I also noticed that Client.ImageTag already parses and normalizes tags given, so we don't need to convert them to their "familiar" form, other than for printing the message;

https://github.com/moby/moby/blob/b4bdf12daec84caaf809a639f923f7370d4926ad/client/image_tag.go#L11-L37

With that taken into account, trust.TrustedPush has no real value, other than printing an informational message, so removing the function and inlining it in the locations where it's used.

⚠️ WARNING: looks like the test is still flaky after this change, so it may just be a bad test, or tests affecting each-other (same port, but different config?). That said; these changes may still be ok to include.

- What I did

- How I did it

- How to verify it

- Human readable description for the release notes



- A picture of a cute animal (not mandatory but encouraged)

thaJeztah avatar Mar 08 '25 11:03 thaJeztah

Codecov Report

Attention: Patch coverage is 0% with 13 lines in your changes missing coverage. Please review.

Project coverage is 59.27%. Comparing base (2eec746) to head (1e32cb2).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5905      +/-   ##
==========================================
+ Coverage   59.26%   59.27%   +0.01%     
==========================================
  Files         357      358       +1     
  Lines       29771    29771              
==========================================
+ Hits        17645    17648       +3     
+ Misses      11153    11150       -3     
  Partials      973      973              
🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov-commenter avatar Mar 08 '25 11:03 codecov-commenter

This looks unrelated; some other issue with GitHub?

#2 ERROR: failed to fetch remote https://github.com/docker/cli.git: git stderr:
error: RPC failed; HTTP 504 curl 22 The requested URL returned error: 504
fatal: expected flush after ref listing
: exit status 128
------
 > [internal] load git source https://github.com/docker/cli.git#refs/pull/5905/merge:
0.198 b20aa460db47688497ecfde3433650ab17957c4e	refs/pull/5905/merge
600.1 error: RPC failed; HTTP 504 curl 22 The requested URL returned error: 504
600.1 fatal: expected flush after ref listing
------
ERROR: failed to solve: failed to read dockerfile: failed to fetch remote https://github.com/docker/cli.git: git stderr:
error: RPC failed; HTTP 504 curl 22 The requested URL returned error: 504
fatal: expected flush after ref listing
: exit status 128

thaJeztah avatar Mar 08 '25 12:03 thaJeztah

Yup, looks like that may be it

thaJeztah avatar Mar 08 '25 12:03 thaJeztah

Ugh; looks like it's still happening, so need to dig deeper, or it was just something that was flaky before?

        Stderr:   error pushing plugin: failed to do request: Head "https://registry:5000/v2/plugin-content-trust/blobs/sha256:f8208ac0663752e1e4fa3ccbc2e4640e2d87b6de7c2a0df4482e1420607bb1b3": http: server gave HTTP response to HTTPS client

thaJeztah avatar Mar 08 '25 14:03 thaJeztah

OK, so looks like it's really just some flaky test. I think these changes still make sense to include, but not sure what's causing those tests to be flaky, other than multiple tests affecting each other possibly.

thaJeztah avatar Mar 08 '25 15:03 thaJeztah