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

optionally include descriptions in schema #585

Closed
wants to merge 8 commits into from

Conversation

puertomontt
Copy link
Contributor

@puertomontt puertomontt commented Sep 11, 2024

Description

Add an option to exclude descriptions from generated openapi schema. This option existed for the Cue based generation (pr) but not the protoc-gen-openapi path. This can be used to reduce the size of the crds.

PR where this functionality is used: https://github.com/solo-io/gloo-mesh-enterprise/pull/18641. You can see that the descriptions were removed from the CRD.

Context

The GME CRD chart is massive and approaching the limit that can be stored in a k8s secret. Removing the descriptions reduces the size drastically.

BOT NOTES:
resolves https://github.com/solo-io/gloo-mesh-enterprise/issues/18562

@solo-changelog-bot
Copy link

Issues linked to changelog:
https://github.com/solo-io/gloo-mesh-enterprise/issues/18562

jenshu
jenshu previously approved these changes Sep 11, 2024
@@ -0,0 +1,6 @@
changelog:
- type: NON_USER_FACING
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: i think this could be NEW_FEATURE although i don't know if we publish the skv2 changelogs anywhere so this is probably fine

@@ -19,12 +19,14 @@ import (

// Implementation of JsonSchemaGenerator that uses a plugin for the protocol buffer compiler
type protocGenerator struct {
validationSchemaOptions *ValidationSchemaOptions
validationSchemaOptions *ValidationSchemaOptions
excludeDescriptionsInSchema bool
Copy link
Contributor

Choose a reason for hiding this comment

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

would it make sense to move this field into ValidationSchemaOptions?
then NewProtocGenerator signature doesn't need to change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not related to validation schema specifically, which is why I felt it should be separate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

open to suggestions on how to refactor though.
Maybe a high level options with validationSchemaOpts nested along with the excludeDescriptionsInSchema?

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO the skip descriptions flag does sound like it's related to the validation schema options, as they all go into the openapi schema in the CRD. but i could be mistaken as i'm not too familiar with the usage of these options

@@ -139,7 +139,7 @@ func generateOpenApi(grp model.Group, protoDir string, protoOpts protoutil.Optio
}

// Use protoc-gen-openapi for transpiling protobuf schemas to openapi v3 with k8s structural schema constraints.
func generateOpenApiFromProtocGen(grp model.Group, protoDir string, _ protoutil.Options, groupOptions model.GroupOptions) (model.OpenApiSchemas, error) {
func generateOpenApiFromProtocGen(grp model.Group, _ string, _ protoutil.Options, groupOptions model.GroupOptions) (model.OpenApiSchemas, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason not to remove the 2nd and 3rd arg if they aren't used?

@puertomontt
Copy link
Contributor Author

Going to park this for now. We might try having links to the docs site rather than removing them

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants