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 12 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 7.86517% with 164 lines in your changes missing coverage. Please review.

Project coverage is 59.25%. Comparing base (e558b91) to head (bb474a6).

❌ Your patch status has failed because the patch coverage (7.86%) 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   59.28%   59.25%   -0.03%     
==========================================
  Files         355      356       +1     
  Lines       29758    29776      +18     
==========================================
+ Hits        17643    17645       +2     
- Misses      11143    11158      +15     
- Partials      972      973       +1     
🚀 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 <github@gone.nl>
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 <github@gone.nl>
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 <github@gone.nl>
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 <github@gone.nl>
@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 <github@gone.nl>
@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 <github@gone.nl>
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 <github@gone.nl>
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 3 times, most recently from b31de14 to e1d1166 Compare March 5, 2025 17:23
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 added 10 commits March 5, 2025 21:36
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>
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 <github@gone.nl>
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 <github@gone.nl>
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 <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This prevents cli-plugins having to import the plugin-manager.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
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 <github@gone.nl>
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