cli/trust: TagTrusted: lazily request APIClient #5905
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention: Patch coverage is
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:
|
thaJeztah
commented
Mar 8, 2025
cli/command/container/create.go
Outdated
Comment on lines
244
to
246
| if taggedRef, ok := namedRef.(reference.NamedTagged); ok && trustedRef != nil { | ||
| return trust.TagTrusted(ctx, dockerCli.Client(), dockerCli.Err(), trustedRef, taggedRef) | ||
| return image.TagTrusted(ctx, dockerCli, trustedRef, taggedRef) | ||
| } |
There was a problem hiding this comment.
Looks like it passes with this revert; wondering if it's because the .Client() would be called as part of the TagTrusted, but now we're calling it early 🤔 (possibly before things related to trust were initialised?)
There was a problem hiding this comment.
☝️ So doesn't look like the cause, and very likely just a flaky test in general.
|
This looks unrelated; some other issue with GitHub? |
|
Yup, looks like that may be it |
thaJeztah
force-pushed
the
test_failures
branch
3 times, most recently
from
94615bb to
39caa02
Compare
thaJeztah
changed the title
|
Ugh; looks like it's still happening, so need to dig deeper, or it was just something that was flaky before? |
|
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
changed the title
thaJeztah
added
kind/refactor
Commit e37d814 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. Signed-off-by: Sebastiaan van Stijn <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Seeing various test-failures that could be a regression, related to
cli/trust: TagTrusted: remove, and inline code
Commit e37d814 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;
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 withthe 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.
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)