🌱 new tag symmetry and required validations #2358
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,7 @@ import ( | |
| "log" | ||
| "os" | ||
| "regexp" | ||
| "slices" | ||
| "strings" | ||
|
|
||
| apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" | ||
|
|
@@ -136,7 +137,7 @@ func runGenerator(args ...string) { | |
| if channel == StandardChannel && strings.Contains(version.Name, "alpha") { | ||
| channelCrd.Spec.Versions[i].Served = false | ||
| } | ||
| channelCrd.Spec.Versions[i].Schema.OpenAPIV3Schema.Properties = opconTweaksMap(channel, channelCrd.Spec.Versions[i].Schema.OpenAPIV3Schema) | ||
| } | ||
|
|
||
| conv, err := crd.AsVersion(*channelCrd, apiextensionsv1.SchemeGroupVersion) | ||
|
|
@@ -179,25 +180,51 @@ func runGenerator(args ...string) { | |
| } | ||
| } | ||
|
|
||
| // Apply Opcon specific tweaks to all properties in a map, and update the parent schema's required list according to opcon tags. | ||
| // For opcon validation optional/required tags, the parent schema's required list is mutated directly. | ||
| func opconTweaksMap(channel string, parentSchema *apiextensionsv1.JSONSchemaProps) map[string]apiextensionsv1.JSONSchemaProps { | ||
| props := parentSchema.Properties | ||
|
|
||
| for name := range props { | ||
| jsonProps := props[name] | ||
| p, reqStatus := opconTweaks(channel, name, jsonProps) | ||
| if p == nil { | ||
| delete(props, name) | ||
| } else { | ||
| props[name] = *p | ||
| // Update required list based on tag | ||
| switch reqStatus { | ||
| case statusRequired: | ||
| if !slices.Contains(parentSchema.Required, name) { | ||
| parentSchema.Required = append(parentSchema.Required, name) | ||
| } | ||
| case statusOptional: | ||
| parentSchema.Required = slices.DeleteFunc(parentSchema.Required, func(s string) bool { return s == name }) | ||
| default: | ||
| // "" (unspecified) means keep existing status | ||
| } | ||
| } | ||
| } | ||
| return props | ||
| } | ||
|
|
||
| // Custom Opcon API Tweaks for tags prefixed with `<opcon:` that get past | ||
| // the limitations of Kubebuilder annotations. | ||
| // Returns the modified schema and a string indicating required status where indicated by opcon tags: | ||
| // "required", "optional", or "" (no decision -- preserve any non-opcon required status). | ||
|
Contributor
There was a problem hiding this comment. instead of strings, would it make sense to use a
Contributor
There was a problem hiding this comment.
Comment on lines
+213
to
+214
Contributor
There was a problem hiding this comment. this is the documentatino for |
||
|
|
||
| const ( | ||
| statusRequired = "required" | ||
| statusOptional = "optional" | ||
| statusNoOpinion = "" | ||
| ) | ||
|
|
||
| func opconTweaks(channel string, name string, jsonProps apiextensionsv1.JSONSchemaProps) (*apiextensionsv1.JSONSchemaProps, string) { | ||
| requiredStatus := statusNoOpinion | ||
|
|
||
| if channel == StandardChannel { | ||
| if strings.Contains(jsonProps.Description, "<opcon:experimental>") { | ||
| return nil, statusNoOpinion | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -219,7 +246,7 @@ func opconTweaks(channel string, name string, jsonProps apiextensionsv1.JSONSche | |
|
|
||
| numValid++ | ||
| jsonProps.Enum = []apiextensionsv1.JSON{} | ||
| for val := range strings.SplitSeq(enumMatch[1], ";") { | ||
| jsonProps.Enum = append(jsonProps.Enum, apiextensionsv1.JSON{Raw: []byte("\"" + val + "\"")}) | ||
| } | ||
| } | ||
|
|
@@ -237,6 +264,21 @@ func opconTweaks(channel string, name string, jsonProps apiextensionsv1.JSONSche | |
| Rule: celMatch[2], | ||
| }) | ||
| } | ||
| optReqRe := regexp.MustCompile(validationPrefix + "(Optional|Required)>") | ||
| optReqMatches := optReqRe.FindAllStringSubmatch(jsonProps.Description, 64) | ||
| for _, optReqMatch := range optReqMatches { | ||
| if len(optReqMatch) != 2 { | ||
| log.Fatalf("Invalid %s Optional/Required tag for %s", validationPrefix, name) | ||
| } | ||
|
|
||
| numValid++ | ||
| switch optReqMatch[1] { | ||
| case "Optional": | ||
| requiredStatus = statusOptional | ||
| case "Required": | ||
| requiredStatus = statusRequired | ||
| } | ||
| } | ||
|
Comment on lines
+267
to
+281
There was a problem hiding this comment. [nitpick] If a field description contains multiple Optional/Required tags (e.g., both
Contributor
There was a problem hiding this comment. Yeah, we should error if we try to set this a second time. |
||
| } | ||
|
|
||
| if numValid < numExpressions { | ||
|
|
@@ -246,34 +288,43 @@ func opconTweaks(channel string, name string, jsonProps apiextensionsv1.JSONSche | |
| jsonProps.Description = formatDescription(jsonProps.Description, channel, name) | ||
|
|
||
| if len(jsonProps.Properties) > 0 { | ||
| jsonProps.Properties = opconTweaksMap(channel, &jsonProps) | ||
| } else if jsonProps.Items != nil && jsonProps.Items.Schema != nil { | ||
| jsonProps.Items.Schema, _ = opconTweaks(channel, name, *jsonProps.Items.Schema) | ||
| } | ||
|
|
||
| return &jsonProps, requiredStatus | ||
| } | ||
|
|
||
| func formatDescription(description string, channel string, name string) string { | ||
| tagset := []struct { | ||
| channel string | ||
| tag string | ||
| }{ | ||
| {channel: ExperimentalChannel, tag: "opcon:standard:description"}, | ||
| {channel: StandardChannel, tag: "opcon:experimental:description"}, | ||
| } | ||
| for _, ts := range tagset { | ||
| startTag := fmt.Sprintf("<%s>", ts.tag) | ||
| endTag := fmt.Sprintf("</%s>", ts.tag) | ||
| if channel == ts.channel && strings.Contains(description, ts.tag) { | ||
| regexPattern := `\n*` + regexp.QuoteMeta(startTag) + `(?s:(.*?))` + regexp.QuoteMeta(endTag) + `\n*` | ||
| re := regexp.MustCompile(regexPattern) | ||
| match := re.FindStringSubmatch(description) | ||
| if len(match) != 2 { | ||
| log.Fatalf("Invalid %s tag for %s", startTag, name) | ||
| } | ||
| description = re.ReplaceAllString(description, "\n\n") | ||
| } else { | ||
| description = strings.ReplaceAll(description, startTag, "") | ||
| description = strings.ReplaceAll(description, endTag, "") | ||
| } | ||
| } | ||
|
|
||
| // Comments within "opcon:util:excludeFromCRD" tag are not included in the generated CRD and all trailing \n operators before | ||
| // and after the tags are removed and replaced with three \n operators. | ||
| startTag := "<opcon:util:excludeFromCRD>" | ||
| endTag := "</opcon:util:excludeFromCRD>" | ||
| if strings.Contains(description, startTag) { | ||
| regexPattern := `\n*` + regexp.QuoteMeta(startTag) + `(?s:(.*?))` + regexp.QuoteMeta(endTag) + `\n*` | ||
| re := regexp.MustCompile(regexPattern) | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 was originally thinking that we'd pass
parentSchemaall the way down intoopconTweaks(which I ultimately still think is the correct architectural decision).However, I see that this function already had some pre-existing code for handling
p == nil(or not) and making changes in the parent at this level.With that in mind, I think your approach makes sense to align with the current architecture.
Maybe leave a comment that it might be worth refactoring in the future to allow
opconTweaksto make any/all necessary changes to the parent to account for the markers that are on thename-ed field?