cli
cli copied to clipboard
cli/trust: TagTrusted: lazily request APIClient
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)
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.
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
Yup, looks like that may be it
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
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.