-
Notifications
You must be signed in to change notification settings - Fork 14
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
Add option to disable kubebuilder markers #555
Conversation
Issues linked to changelog: |
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.
nice work!
@@ -32,7 +32,7 @@ require ( | |||
github.com/solo-io/go-utils v0.24.8 | |||
github.com/solo-io/k8s-utils v0.7.2 | |||
github.com/solo-io/protoc-gen-ext v0.0.18 | |||
github.com/solo-io/protoc-gen-openapi v0.1.1 | |||
github.com/solo-io/protoc-gen-openapi v0.2.4 |
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.
nit: When bumping libraries across minor versions, I tend to callout in the PR body what those changes were. Mostly just to get in the habit of validating changes, because we have noticed that sometimes there are behavioral change that impact us. Since this is an internal library that we manage, and is low risk, it's certainly not a requirement (more sharing as it's a pattern I follow)
@@ -105,10 +109,11 @@ func (o *OpenApiProtocExecutor) Execute(protoFile string, toFile string, imports | |||
_ = os.Mkdir(directoryPath, os.ModePerm) | |||
|
|||
cmd.Args = append(cmd.Args, | |||
fmt.Sprintf("--openapi_out=yaml=true,single_file=false,include_description=%v,enum_as_int_or_string=%v,additional_empty_schema=%v:%s", | |||
fmt.Sprintf("--openapi_out=yaml=true,single_file=false,include_description=%v,enum_as_int_or_string=%v,additional_empty_schema=%v,disable_kube_markers=%v:%s", |
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 like this change, because it follows the precedent that existed. An alternative way to perhaps improve this "API" would be to just expose a raw string to users. This string would be passed from callers of the code, down to protoc. This way, the next time we add an argument to our open-api protoc impelementation, we don't need to pipe through values, and can instead just update the 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'm not sure I prefer that to exposing a clear option in solo-kit that IDEs won't let you typo and such, but certainly a conversation we can have.
Description
Adds option to disable kubebuilder markers and validations to omit them from the generated OpenAPI schema.
Changes can be seen in-action here
Context
This is needed for https://github.com/solo-io/gloo-mesh-enterprise/issues/17005
When disableKubeMarkers is set to true,
Type=object
annotations will be respected such that recursive fields will still work.Required
annotations will also be respected.This is essentially a cherry-pick of solo-io/skv2#567.