Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cli/command: remove NotaryClient from CLI interface #5876

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Mar 1, 2025

cli/command: remove deprecated NotaryClient from CLI interface

This method is a shallow wrapper around trust.GetNotaryRepository, but
due to its signature resulted in the trust package, and notary dependencies
to become a dependency of the CLI. Consequence of this was that cli-plugins,
which need the cli/command package, would also get notary and its
dependencies as a dependency. It is no longer used, and was deprecated
in 9bc16bb.

This patch removes the NotaryClient method from the interface

cli/command: remove deprecated ManifestStore from CLI interface

This method is a shallow wrapper around manifeststore.NewStore, but
due to its signature resulted in various dependencies becoming a dependency
of the "command" package. Consequence of this was that cli-plugins, which
need the cli/command package, would also get those dependencies. It is no
longer used, and was deprecated in e32d5d5.

This patch removes the ManifestStore method from the interface

cli/command: remove deprecated RegistryClient from CLI interface

This method was a shallow wrapper around registryclient.NewRegistryClient but
due to its signature resulted in various dependencies becoming a dependency
of the "command" package. Consequence of this was that cli-plugins, which
need the cli/command package, would also get those dependencies. It is no
longer used, and was deprecated in 8ad0721.

This patch removes the RegistryClient method from the interface

- Human readable description for the release notes

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

@thaJeztah thaJeztah added status/2-code-review area/trust kind/refactor PR's that refactor, or clean-up code labels Mar 1, 2025
@codecov-commenter
Copy link

codecov-commenter commented Mar 1, 2025

Codecov Report

Attention: Patch coverage is 34.74576% with 77 lines in your changes missing coverage. Please review.

Project coverage is 59.29%. Comparing base (60ae1bb) to head (25d574f).
Report is 2 commits behind head on master.

❌ Your patch status has failed because the patch coverage (34.74%) 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    #5876      +/-   ##
==========================================
+ Coverage   58.89%   59.29%   +0.39%     
==========================================
  Files         355      358       +3     
  Lines       29761    29811      +50     
==========================================
+ Hits        17529    17677     +148     
+ Misses      11258    11164      -94     
+ Partials      974      970       -4     
🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

thaJeztah added a commit to thaJeztah/buildx that referenced this pull request Mar 1, 2025
Testing docker/cli#5876 to remove notary
dependency.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah added a commit to thaJeztah/buildx that referenced this pull request Mar 1, 2025
Testing docker/cli#5876 to remove notary
dependency.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah added a commit to thaJeztah/buildx that referenced this pull request Mar 1, 2025
Testing docker/cli#5876 to remove notary
dependency.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah added a commit to thaJeztah/buildx that referenced this pull request Mar 1, 2025
Testing docker/cli#5876 to remove notary
dependency.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah force-pushed the less_notary branch 3 times, most recently from 0939e55 to 331753a Compare March 2, 2025 13:20
thaJeztah added a commit to thaJeztah/buildx that referenced this pull request Mar 2, 2025
Testing docker/cli#5876 to remove notary
dependency.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah force-pushed the less_notary branch 3 times, most recently from e017b03 to 749bea0 Compare March 3, 2025 11:51
thaJeztah added a commit to thaJeztah/buildx that referenced this pull request Mar 3, 2025
Testing docker/cli#5876 to remove notary
dependency.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah added a commit to thaJeztah/buildx that referenced this pull request Mar 4, 2025
Testing docker/cli#5876 to remove notary
dependency.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
Comment on lines 51 to 50
hooks.PrintNextSteps(dockerCli.Err(), nextSteps)
hooks.PrintNextSteps(rootCmd.ErrOrStderr(), nextSteps)
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a difference; before this, we would take the Err() output from the CLI, and now we switched to the Cobra cmd's StdErr() - we should look in our code altogether to consider using that (instead of depending on the DockerCLI to provide us the stderr/stdout; I think they're coupled either way.

@thaJeztah thaJeztah force-pushed the less_notary branch 5 times, most recently from 27de409 to bb474a6 Compare March 5, 2025 21:09
thaJeztah added a commit to thaJeztah/buildx that referenced this pull request Mar 5, 2025
Testing docker/cli#5876 to remove notary
dependency.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah force-pushed the less_notary branch 4 times, most recently from a3dcdf9 to 2e3b00b Compare March 8, 2025 16:17
thaJeztah added a commit to thaJeztah/buildx that referenced this pull request Mar 8, 2025
Testing docker/cli#5876 to remove notary
dependency.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah added a commit to thaJeztah/buildx that referenced this pull request Mar 8, 2025
Testing docker/cli#5876 to remove notary
dependency.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah added a commit to thaJeztah/buildx that referenced this pull request Mar 10, 2025
Testing docker/cli#5876 to remove notary
dependency.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah added a commit to thaJeztah/buildx that referenced this pull request Mar 10, 2025
Testing docker/cli#5876 to remove notary
dependency.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah added a commit to thaJeztah/buildx that referenced this pull request Mar 10, 2025
Testing docker/cli#5876 to remove notary
dependency.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
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]>
This method is a shallow wrapper around trust.GetNotaryRepository, but
due to its signature resulted in the trust package, and notary dependencies
to become a dependency of the CLI. Consequence of this was that cli-plugins,
which need the cli/command package, would also get notary and its
dependencies as a dependency. It is no longer used, and was deprecated
in 9bc16bb.

This patch removes the NotaryClient method from the interface

Signed-off-by: Sebastiaan van Stijn <[email protected]>
This method is a shallow wrapper around manifeststore.NewStore, but
due to its signature resulted in various dependencies becoming a dependency
of the "command" package. Consequence of this was that cli-plugins, which
need the cli/command package, would also get those dependencies. It is no
longer used, and was deprecated in e32d5d5.

This patch removes the ManifestStore method from the interface

Signed-off-by: Sebastiaan van Stijn <[email protected]>
This method was a shallow wrapper around registryclient.NewRegistryClient but
due to its signature resulted in various dependencies becoming a dependency
of the "command" package. Consequence of this was that cli-plugins, which
need the cli/command package, would also get those dependencies. It is no
longer used, and was deprecated in 8ad0721.

This patch removes the RegistryClient method from the interface

Signed-off-by: Sebastiaan van Stijn <[email protected]>
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
Labels
area/trust kind/refactor PR's that refactor, or clean-up code status/2-code-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants