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, cil/command/image: remove deprecated methods and functions #5876

Open
wants to merge 6 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

cli/command/image: remove deprecated TrustedPush

This function was only used by "docker trust sign", and has no known external
consumers. It was deprecated in c6f456b;
this commit removes it.

cli/command/image: remove deprecated PushTrustedReference

This function was only used internally, and has no known external consumers.
It was deprecated in d804360; this commit
removes it.

cli/command/image: remove deprecated TagTrusted

This function was only used internally, and has no known external consumers.
It was deprecated in e37d814; this commit
removes it.

- Human readable description for the release notes

Go-SDK: `cli/command/image`: remove deprecated `TrustedPush`, `TagTrusted`, and `PushTrustedReference` functions.
Go-SDK: `cli/command`: remove deprecated `NotaryClient` and `ManifestStore`, and `RegistryClient` from CLI interface.

- 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

All modified and coverable lines are covered by tests ✅

Project coverage is 59.46%. Comparing base (e002576) to head (4541df2).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5876      +/-   ##
==========================================
+ Coverage   59.42%   59.46%   +0.04%     
==========================================
  Files         358      357       -1     
  Lines       29768    29747      -21     
==========================================
  Hits        17690    17690              
+ Misses      11113    11092      -21     
  Partials      965      965              
🚀 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 10, 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 7 times, most recently from 88fe1f9 to 0a60a49 Compare March 11, 2025 09:12
@thaJeztah thaJeztah force-pushed the less_notary branch 4 times, most recently from c5dad66 to 45c0ebe Compare March 19, 2025 17:57
@thaJeztah thaJeztah changed the title cli/command: remove NotaryClient from CLI interface cli/command, cil/command/image: remove deprecated methods and functions Mar 19, 2025
@thaJeztah thaJeztah added impact/deprecation impact/go-sdk Noteworthy (compatibility changes) in the Go SDK labels Mar 19, 2025
@thaJeztah thaJeztah added this to the 28.0.3 milestone Mar 19, 2025
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]>
This function was only used by "docker trust sign", and has no known external
consumers. It was deprecated in c6f456b;
this commit removes it.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
This function was only used internally, and has no known external consumers.
It was deprecated in d804360; this commit
removes it.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
This function was only used internally, and has no known external consumers.
It was deprecated in e37d814; this commit
removes it.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah marked this pull request as ready for review March 20, 2025 12:34
thaJeztah added a commit to thaJeztah/buildx that referenced this pull request Mar 20, 2025
Testing docker/cli#5876 to remove notary
dependency.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
Copy link
Contributor

@tianon tianon left a comment

Choose a reason for hiding this comment

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

Surprising nobody (and I'm not a maintainer here, so FWIW), this LGTM 😇

@thaJeztah
Copy link
Member Author

Thanks both! I'll wait for others to have a last look Tomorrow before merging, but looks like we can go ahead.

For others visiting this PR; removing these methods technically is a "breaking" change, so would require a major version update.

However, the cli/command package was never really designed as a public API, and gained features and functions that were ultimately for "internal use".

The features removed here specifically were intended for internal use in the CLI itself, and not for external consumers, but had a big impact on every (cli-plugin) user that used the docker/cli as dependency.

I did a search through public repositories, and could not find any consumer of these functions (and I would not expect any user of out code to actually be using these), so removing these should very unlikely impact any user of the code, other than in a "positive" way of many (indirect) dependencies being removed 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/trust impact/deprecation impact/go-sdk Noteworthy (compatibility changes) in the Go SDK 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.

4 participants