api/types: fix Plugin.Config.Interface.Types Swagger definition#50519
api/types: fix Plugin.Config.Interface.Types Swagger definition#50519thaJeztah merged 2 commits intomoby:masterfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the Swagger definition for Plugin.Config.Interface.Types to properly represent it as an array of strings in the API while maintaining structured type information in Go code. The change addresses issues with plugin interface type representation and updates the go-swagger tooling to support external type extensions.
Key changes:
- Updated Swagger definition to use array of strings with external Go type mapping
- Created new
CapabilityIDtype inpkg/plugins/pluginmetapackage with proper marshaling/unmarshaling - Upgraded go-swagger version and modified generation templates
- Updated all references throughout codebase to use the new type
Reviewed Changes
Copilot reviewed 40 out of 72 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| api/swagger.yaml | Updated plugin interface types definition to use string array with external type mapping |
| pkg/plugins/pluginmeta/capability.go | New CapabilityID type with text marshaling/unmarshaling implementation |
| pkg/plugins/pluginmeta/capability_test.go | Comprehensive tests for the new CapabilityID type |
| api/types/plugin.go | Updated to use new CapabilityID type and added import |
| api/types/plugin_interface_type.go | Removed old PluginInterfaceType definition |
| api/types/plugin_responses.go | Removed marshaling/unmarshaling methods for old type |
| hack/generate-swagger-api.sh | Updated swagger generation commands and removed old type |
| Dockerfile | Upgraded go-swagger version to v0.32.3 |
| Multiple test files | Updated to use new CapabilityID type |
| Multiple generated files | Updated with new swagger generation output |
c572aa9 to
f1cd616
Compare
b1ac092 to
efdb0e3
Compare
|
I tried putting the CapabilityID definition into pkg/plugins but that would lead to a module dependency cycle (github.com/moby/moby -> github.com/moby/moby/api -> github.com/moby/moby) so I took the path of least resistance and moved the type definition back into the api module. |
4c47558 to
c1c0aab
Compare
c1c0aab to
9de1602
Compare
|
do we want to do the move of these types to api/types/plugin (and remove the "Plugin" prefix) as well as part of this PR, or in a follow up? |
|
@thaJeztah I structured this PR to not conflict with your PR that moves the plugin type definitions. I say we keep the PRs separate |
|
Gotcha! Happy to do the follow up; I wasn't sure if it was intentional. @corhere this one needs a rebase now (as the module PR was merged) |
Override some of the templates to suppress emitting unwanted validation and marshal/unmarshal code. Signed-off-by: Cory Snider <csnider@mirantis.com>
The wire type of Plugin.Config.Interface.Types is an array of strings, not of objects with three properties. We just so happen to have a Go struct type to represent a plugin-interface-type value in memory with all the fields parsed out for convenience, but that is not part of the REST API contract documented by the Swager spec.U pdate the Swagger spec to correctly document that the Types property is an array of strings in the API, while still generating Go definitions that unmarshal into the convenient struct type. Move the definition and marshal/unmarshal methods for PluginInterfaceType into a more appropriate location than api/types. Rename the type to one that does not stutter or overload already heavily overloaded terminology. Modernize the parser and use property-based testing to assert that it behaves the same as the old parser for all well-formed inputs. Signed-off-by: Cory Snider <csnider@mirantis.com>
9de1602 to
ee560a3
Compare
thaJeztah
left a comment
There was a problem hiding this comment.
LGTM
Thank YOU! Really happy we finally got rid of that swagger fork 🎉
| // Assert that the value can be round-tripped successfully. | ||
| if strings.Contains(t.Capability, ".") { | ||
| return nil, fmt.Errorf("capability %q cannot contain a dot", t.Capability) | ||
| } |
There was a problem hiding this comment.
Interesting; getting a failure in some e2e test in the CLI - not unlikely that the dummy value it used was supposed to be valid 🤔 ;
=== Failed
=== FAIL: e2e/global TestPromptExitCode/plugin_install (0.28s)
cli_test.go:203: assertion failed: error is not nil: json: error calling MarshalText for type plugin.CapabilityID: capability "docker.dummy/1.0" cannot contain a dot
=== FAIL: e2e/global TestPromptExitCode/plugin_upgrade (0.26s)
cli_test.go:203: assertion failed: error is not nil: json: error calling MarshalText for type plugin.CapabilityID: capability "docker.dummy/1.0" cannot contain a dot
There was a problem hiding this comment.
Yes; PluginInterfaceType was frequently misused by setting the Capability field to the fully-qualified capability ID and leaving the other two fields blank. A value such as PluginInterfaceType{Capability: "docker.dummy/1.0"} would marshal to the string ".docker.dummy/1.0/" which is not well-formed and does not round-trip to the same value even with the old parser
There was a problem hiding this comment.
Yes, I later realised that was probably what happened.
In all honesty, I don't think there will be many (if any) users of these types; plugins are not widely used, and I doubt there's many users that consume that part of the client (and API).
- What I did
I fixed up the Swagger definition for
Plugin.Config.Interface.Typesto accurately express that it is an array of strings over the wire while maintaining the parsed struct type on the Go side.- How I did it
I upgraded to the latest go-swagger to make use of
x-go-typeexternal type extensions so the generated Go struct field uses the type of my choice instead of plain strings. I customized the Swagger generation templates to suppress generating validators or marshal/unmarshal code.- How to verify it
CI
- Human readable description for the release notes
- A picture of a cute animal (not mandatory but encouraged)