-
Notifications
You must be signed in to change notification settings - Fork 2k
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
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
❌ Your patch status has failed because the patch coverage (6.66%) 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.94% 59.51% +0.56%
==========================================
Files 355 358 +3
Lines 29772 29705 -67
==========================================
+ Hits 17550 17678 +128
+ Misses 11251 11062 -189
+ Partials 971 965 -6 🚀 New features to boost your workflow:
|
Testing docker/cli#5876 to remove notary dependency. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Testing docker/cli#5876 to remove notary dependency. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Testing docker/cli#5876 to remove notary dependency. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Testing docker/cli#5876 to remove notary dependency. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
0939e55
to
331753a
Compare
Testing docker/cli#5876 to remove notary dependency. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
e017b03
to
749bea0
Compare
Testing docker/cli#5876 to remove notary dependency. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Testing docker/cli#5876 to remove notary dependency. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
cli-plugins/manager/hooks.go
Outdated
hooks.PrintNextSteps(dockerCli.Err(), nextSteps) | ||
hooks.PrintNextSteps(rootCmd.ErrOrStderr(), nextSteps) |
There was a problem hiding this comment.
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.
27de409
to
bb474a6
Compare
Testing docker/cli#5876 to remove notary dependency. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Testing docker/cli#5876 to remove notary dependency. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Testing docker/cli#5876 to remove notary dependency. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Testing docker/cli#5876 to remove notary dependency. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
20a4711
to
88fe1f9
Compare
full diff: moby/moby@v28.0.1...5f0d673 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Starting with [moby@b633c4c], the registry package handles this internally and there's no longer a need to set the path manually for rootlessKit [moby@b633c4c]: moby/moby@b633c4c Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This validation is now handled by the API-client since [moby@5d6b566], so no longer needed to be done in the cli. This function was only used internally and has no external consumers, so removing it without deprecating first. [moby@5d6b566]: moby/moby@5d6b566 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Since [moby@c2c3d59], [registry.ParseRepositoryInfo] now always returns a nil error, so we can remove the error handling. [registry.ParseRepositoryInfo]: https://github.com/moby/moby/blob/5f0d6731eb015c8e46b2ae9bb1803d005f2513be/registry/config.go#L414-L443 [moby@c2c3d59]: moby/moby@c2c3d59 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
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 <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>
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>
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)