-
Notifications
You must be signed in to change notification settings - Fork 2.1k
cli/command: add WithUserAgent option #4574
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
Conversation
cli-plugins/manager/metadata.go
Outdated
| case "0.2.0": | ||
| if meta.Name == "" { |
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.
We should be careful with this one (yes, I've been looking at that gnarly "must be 0.1.0" as well), but if we'd ship a plugin with 0.2.0, it would mean it cannot be used with any CLI other than docker 25.0.0
Which also means that anyone installing an older version of the deb/rpm packages will now be broken.
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.
Gah! You're absolutely right.
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.
I guess the 0.1.0 threw me for a loop and made me think "oh, semver, I should update this!"
I suppose from the perspective of a JSON-ish API, field additions are typically acceptable from a compatibility standpoint. How do you feel if I rollback the SchemaVersion part? Keep it as 0.1.0 but start sending Name.
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.
A possible alternative: change the signature of plugin.Run() to be (name, meta, factoryFn). It avoids changing Metadata at all at the expense of breaking the API. (That will guarantee that all plugins start reporting UA once they upgrade at least...)
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.
So, I think we can change the code to remove the fixed requirement (we can set a minimum version of v0.1.0)
That won't fix existing cli's, but at least removes the weird bit for future.
Other than that, I think it's fine to add new fields, which means that we can detect if new fields are set (older CLI's would ignore them), and we could use that for feature detection.
(possibly a field that indicates what features are supported, if that helps avoid ambiguity between "empty field" and "feature supported but some field is empty")
| dockerEndpoint docker.Endpoint | ||
| contextStoreConfig store.Config | ||
| initTimeout time.Duration | ||
| userAgent string |
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.
- I recall I opened client: add WithUserAgent() option moby/moby#45677 to add an option to the APIClient; wondering if we can punch through to that one, without also having to store it in the DockerCLI
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.
DockerCli uses that helper for API client construction! But DockerCli also lazily creates RegistryClient and NotaryClient instances and passes along a user agent to them, so it does need it.
I agree that we might want to add a WithAPIClientOption passthrough for future tweaks to the API client without needing 2x PRs each time
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.
Ah! Right, yeah, I hate it that we also re-invent our own "registry" client here (and I think all that 💩 is for, erm, mostly legacy stuff).
I'll have a closer look if I can come up with some solutions (otherwise this looks fine to me). Things I still have my eyes on (not related to your changes per-se, but a bit of a sore point already);
- not a fan of requiring a fixed argument to pass around "user-agent" everywhere
- ☝️ even more if that is "hard-coded" (hard-coded, as in: a function-call that is hard-coded)
- ☝️ at least your PR improves that part
But indeed persisting in 2 separate locations isn't ideal; also considering; is this something we should pass over the context (at least that would allow passing this around without requiring it to be passed as a dedicated argument that "somehow" needs to find its way somewhere)
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.
Clarification: when you say context, do you mean Docker context or Go context.Context?
It could canonically live inside the API client, but then that interface would need to gain a getter so that DockerCli could read it back out to pass to registry & notary client.
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.
Doh! Sorry, somehow missed your last comment (also still need to look at this PR again 🙈)
I meant context.Context and to (ab)use the context to pass this information, which would allow functions that need a user-agent to get access to it (which could potentially avoid having to punch-through a UserAgent argument to many places.
I haven't given that idea much thought yet though; I was considering that approach for API version (but for API version it may be more "clear-cut" that the API version to use would usually be "per request", and as such "per context"; for User-Agent that's more of a "grey area" I guess 😂
4d1bf8b to
70e7426
Compare
cli-plugins/plugin/plugin.go
Outdated
| var dockerCliOpts []command.DockerCliOption | ||
| if ua, ok := userAgent(meta); ok { | ||
| dockerCliOpts = append(dockerCliOpts, command.WithUserAgent(ua)) | ||
| } | ||
|
|
||
| dockerCli, err := command.NewDockerCli(dockerCliOpts...) |
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.
Sorry for the delay (again), this one dropped off my list.
I was just wondering; do we actually need the Metadata to have the new field? Would it also work if the plugin would use dockerCli.Apply() and set the user agent?
Looking at compose; https://github.com/docker/compose/blob/e5cd265abbdd9d67d76558d3b8d2e7fec68b7573/cmd/main.go#L36-L39
It gets the dockerCLI passed; which (with the option you added in this PR) could use that function to set a custom user-agent;
func pluginMain() {
plugin.Run(func(dockerCli command.Cli) *cobra.Command {
_ = dockerCli.Apply(command.WithUserAgent("whatever"))The net-result would be the same, but without the Metadata as intermediate (because that one also gets used for the JSON response, where it's not used?)
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
@thaJeztah PTAL - I refactored and there's no changes in the plugin metadata needed any longer. I'm using the [See updated PR description for more details] |
0bfe13b to
2b03789
Compare
2b03789 to
f278cc1
Compare
Add support to the `cli/command` package to accept a custom User Agent to pass to the underlying client. This is used as the `UpstreamClient` portion of the `User-Agent` when the Moby daemon makes requests. For example, pushing and pulling images with Compose might result in the registry seeing a `User-Agent` value of: ``` docker/24.0.7 go/go1.20.10 git-commit/311b9ff kernel/6.5.13-linuxkit os/linux arch/arm64 UpstreamClient(docker-cli-plugin-compose/v2.24.0) ``` Signed-off-by: Milas Bowman <[email protected]> Signed-off-by: Sebastiaan van Stijn <[email protected]>
f278cc1 to
048e931
Compare
thaJeztah
left a comment
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.
LGTM
I rewrote this to take advantage of new options we have on plugin.Run
How I did it
Add support to the
cli/commandpackage to accept a custom UserAgent to pass to the underlying client.
This is used as the
UpstreamClientportion of theUser-Agentwhen the Moby daemon makes requests.
For example, pushing and pulling images with Compose might result
in the registry seeing a
User-Agentvalue of:See also:
- How to verify it
Build a CLI plugin against latest
HEAD, update itsSchemaVersionto0.2.0, and inspect network traffic.- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)
