Skip to content

move some trust-related code to trust package - #5894

Merged
thaJeztah merged 3 commits into
docker:masterfrom
thaJeztah:more_internalize
Mar 6, 2025
Merged

move some trust-related code to trust package#5894
thaJeztah merged 3 commits into
docker:masterfrom
thaJeztah:more_internalize

Conversation

@thaJeztah

@thaJeztahthaJeztah commented Mar 5, 2025

Copy link
Copy Markdown
Member

cli/command/image: deprecate and internalize TrustedPush

This function was only used by "docker trust sign"; inline the code
and deprecate the function.

This function has no known external consumers, so we should remove
it on the first possible ocassion (which could be a minor release).

cli/command/image: deprecate PushTrustedReference, move to trust

This function was shared between "trust" "image" and "plugin" packages,
all of which needed the trust package, so move it there instead.

cli/command/image: deprecate TagTrusted, move to cli/trust

This function was shared between "image" and "container" packages,
all of which needed the trust package, so move it there instead.

- Human readable description for the release notes

Go-SDK: `cli/command/image`: Deprecate and internalize `TrustedPush`.
Go-SDK: `cli/command/image`: Deprecate `PushTrustedReference` and move to `cli/trust`.

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

@thaJeztahthaJeztah added status/2-code-review area/trust kind/refactor PR's that refactor, or clean-up code impact/changelog impact/deprecation impact/go-sdk Noteworthy (compatibility changes) in the Go SDK labels Mar 5, 2025
@codecov-commenter

codecov-commenter commented Mar 5, 2025

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 2.58621% with 113 lines in your changes missing coverage. Please review.

Project coverage is 59.29%. Comparing base (e558b91) to head (e37d814).
Report is 6 commits behind head on master.

❌ Your patch status has failed because the patch coverage (2.58%) is below the target coverage (50.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@ Coverage Diff @@## master #5894 +/- ##
=======================================
Coverage 59.28% 59.29% =======================================
Files 355 357 +2 Lines 29758 29771 +13 =======================================
+ Hits 17643 17653 +10 - Misses 11143 11144 +1 - Partials 972 974 +2 
🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

This function was only used by "docker trust sign"; inline the code
and deprecate the function.
This function has no known external consumers, so we should remove
it on the first possible ocassion (which could be a minor release).
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This function was shared between "trust" "image" and "plugin" packages,
all of which needed the trust package, so move it there instead.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
thaJeztahforce-pushed the more_internalize branch 3 times, most recently from 283ba0e to 7debfafCompareMarch 5, 2025 17:58
This function was shared between "image" and "container" packages,
all of which needed the trust package, so move it there instead.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
thaJeztah marked this pull request as ready for review March 5, 2025 20:48
Comment on lines +107 to +111
// Use familiar references when interacting with client and output
familiarRef := reference.FamiliarString(tagged)
trustedFamiliarRef := reference.FamiliarString(trustedRef)
_, _ = fmt.Fprintf(cli.Err(), "Tagging %s as %s\n", trustedFamiliarRef, familiarRef)
if err := cli.Client().ImageTag(ctx, trustedFamiliarRef, familiarRef); err != nil {

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW; there's a couple of changes I want to start making; I just had a look at the image references and formats we're using, and, even slightly simplified in my test, we're literally converting the image name 11 times, including juggling between a string, to a reference (various permutations), back to a string, before we even make a request with the engine 🫠

=== RUN TestJuggleFormats STEP 1: busybox (string) STEP 2: docker.io/library/busybox (reference.repository) STEP 3: docker.io/library/busybox:latest (reference.taggedReference) STEP 4: docker.io/library/busybox:latest (string) STEP 5: docker.io/library/busybox:latest (reference.taggedReference) STEP 6A: docker.io/library/busybox@sha256:498a000f370d8c37927118ed80afe8adc38d1edcbfc071627d17b25c88efcab0 (reference.canonicalReference) STEP 6B: docker.io/library/busybox:latest (reference.taggedReference) --> Tagging busybox@sha256:498a000f370d8c37927118ed80afe8adc38d1edcbfc071627d17b25c88efcab0 as busybox:latest STEP 7A: busybox@sha256:498a000f370d8c37927118ed80afe8adc38d1edcbfc071627d17b25c88efcab0 (string) STEP 7B: busybox:latest (string) STEP 8: docker.io/library/busybox:latest (reference.taggedReference) STEP 9: docker.io/library/busybox:latest (reference.taggedReference) --> SENDING TO DAEMON: /images/busybox@sha256:498a000f370d8c37927118ed80afe8adc38d1edcbfc071627d17b25c88efcab0/tag?repo=busybox&tag=latest STEP 10: docker.io/library/busybox (reference.repository) STEP 11: docker.io/library/busybox:latest (reference.taggedReference) STEP 12: busybox (string) --> backend.GetImage --> backend.TagImage

@thaJeztah
thaJeztah merged commit ceef542 into docker:masterMar 6, 2025
@thaJeztah
thaJeztah deleted the more_internalize branch March 6, 2025 17:10
@thaJeztahthaJeztah added this to the 28.0.2 milestone Mar 6, 2025
Comment on lines 245 to 247
if taggedRef, ok := namedRef.(reference.NamedTagged); ok && trustedRef != nil {
return image.TagTrusted(ctx, dockerCli, trustedRef, taggedRef)
return trust.TagTrusted(ctx, dockerCli.Client(), dockerCli.Err(), trustedRef, taggedRef)
}

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this introduced a regression; most likely because before this dockerCli.Client() would be initialised lazily, and it's possible some settings were loaded elsewhere.

I opened a PR that reverts some of the changes;

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like reverting those changes (PR above) still makes things fail occasionally.

I think it's just a bad test and/or multiple tests affecting each other (perhaps using same port or otherwise)

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/trustimpact/changelogimpact/deprecationimpact/go-sdkNoteworthy (compatibility changes) in the Go SDKkind/refactorPR's that refactor, or clean-up codestatus/2-code-review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@thaJeztah@codecov-commenter@vvoland@Benehiko