review updates

Signed-off-by: grokspawn <jordan@nimblewidget.com>

Author: grokspawn

hack/tools/crd-generator/main.go

@@ -137,7 +137,7 @@ func runGenerator(args ...string) {
 				if channel == StandardChannel && strings.Contains(version.Name, "alpha") {
 					channelCrd.Spec.Versions[i].Served = false
 				}
-				version.Schema.OpenAPIV3Schema.Properties, version.Schema.OpenAPIV3Schema.Required = opconTweaksMap(channel, version.Schema.OpenAPIV3Schema.Properties, version.Schema.OpenAPIV3Schema.Required)
+				channelCrd.Spec.Versions[i].Schema.OpenAPIV3Schema.Properties = opconTweaksMap(channel, channelCrd.Spec.Versions[i].Schema.OpenAPIV3Schema)
 			}
 
 			conv, err := crd.AsVersion(*channelCrd, apiextensionsv1.SchemeGroupVersion)
@@ -180,10 +180,11 @@ func runGenerator(args ...string) {
 	}
 }
 
-// Apply Opcon specific tweaks to all properties in a map, and return a list of required fields according to opcon tags.
-// For opcon validation optional/required tags, the required list is updated accordingly.
-func opconTweaksMap(channel string, props map[string]apiextensionsv1.JSONSchemaProps, existingRequired []string) (map[string]apiextensionsv1.JSONSchemaProps, []string) {
-	newRequired := slices.Clone(existingRequired)
+// 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.
+// TODO: if we need to support other conditions from opconTweaks, it will likely be preferable to convey the parent schema to facilitate direct alteration.
+func opconTweaksMap(channel string, parentSchema *apiextensionsv1.JSONSchemaProps) map[string]apiextensionsv1.JSONSchemaProps {
+	props := parentSchema.Properties
 
 	for name := range props {
 		jsonProps := props[name]
@@ -195,30 +196,29 @@ func opconTweaksMap(channel string, props map[string]apiextensionsv1.JSONSchemaP
 			// Update required list based on tag
 			switch reqStatus {
 			case statusRequired:
-				if !slices.Contains(newRequired, name) {
-					newRequired = append(newRequired, name)
+				if !slices.Contains(parentSchema.Required, name) {
+					parentSchema.Required = append(parentSchema.Required, name)
 				}
 			case statusOptional:
-				newRequired = slices.DeleteFunc(newRequired, func(s string) bool { return s == name })
+				parentSchema.Required = slices.DeleteFunc(parentSchema.Required, func(s string) bool { return s == name })
 			default:
 				// "" (unspecified) means keep existing status
 			}
 		}
 	}
-	return props, newRequired
+	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).
-
 const (
 	statusRequired  = "required"
 	statusOptional  = "optional"
 	statusNoOpinion = ""
 )
 
+// 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).
 func opconTweaks(channel string, name string, jsonProps apiextensionsv1.JSONSchemaProps) (*apiextensionsv1.JSONSchemaProps, string) {
 	requiredStatus := statusNoOpinion
 
@@ -266,6 +266,8 @@ func opconTweaks(channel string, name string, jsonProps apiextensionsv1.JSONSche
 		}
 		optReqRe := regexp.MustCompile(validationPrefix + "(Optional|Required)>")
 		optReqMatches := optReqRe.FindAllStringSubmatch(jsonProps.Description, 64)
+		hasOptional := false
+		hasRequired := false
 		for _, optReqMatch := range optReqMatches {
 			if len(optReqMatch) != 2 {
 				log.Fatalf("Invalid %s Optional/Required tag for %s", validationPrefix, name)
@@ -274,11 +276,16 @@ func opconTweaks(channel string, name string, jsonProps apiextensionsv1.JSONSche
 			numValid++
 			switch optReqMatch[1] {
 			case "Optional":
+				hasOptional = true
 				requiredStatus = statusOptional
 			case "Required":
+				hasRequired = true
 				requiredStatus = statusRequired
 			}
 		}
+		if hasOptional && hasRequired {
+			log.Fatalf("Field %s has both Optional and Required validation tags for channel %s", name, channel)
+		}
 	}
 
 	if numValid < numExpressions {
@@ -288,7 +295,7 @@ func opconTweaks(channel string, name string, jsonProps apiextensionsv1.JSONSche
 	jsonProps.Description = formatDescription(jsonProps.Description, channel, name)
 
 	if len(jsonProps.Properties) > 0 {
-		jsonProps.Properties, jsonProps.Required = opconTweaksMap(channel, jsonProps.Properties, jsonProps.Required)
+		jsonProps.Properties = opconTweaksMap(channel, &jsonProps)
 	} else if jsonProps.Items != nil && jsonProps.Items.Schema != nil {
 		jsonProps.Items.Schema, _ = opconTweaks(channel, name, *jsonProps.Items.Schema)
 	}
@@ -299,24 +306,25 @@ func opconTweaks(channel string, name string, jsonProps apiextensionsv1.JSONSche
 func formatDescription(description string, channel string, name string) string {
 	tagset := []struct {
 		channel string
-		start   string
-		end     string
+		tag     string
 	}{
-		{channel: ExperimentalChannel, start: "<opcon:standard:description>", end: "</opcon:standard:description>"},
-		{channel: StandardChannel, start: "<opcon:experimental:description>", end: "</opcon:experimental:description>"},
+		{channel: ExperimentalChannel, tag: "opcon:standard:description"},
+		{channel: StandardChannel, tag: "opcon:experimental:description"},
 	}
 	for _, ts := range tagset {
-		if channel == ts.channel && strings.Contains(description, ts.start) {
-			regexPattern := `\n*` + regexp.QuoteMeta(ts.start) + `(?s:(.*?))` + regexp.QuoteMeta(ts.end) + `\n*`
+		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 <opcon:experimental:description> tag for %s", name)
+				log.Fatalf("Invalid %s tag for %s", startTag, name)
 			}
 			description = re.ReplaceAllString(description, "\n\n")
 		} else {
-			description = strings.ReplaceAll(description, ts.start, "")
-			description = strings.ReplaceAll(description, ts.end, "")
+			description = strings.ReplaceAll(description, startTag, "")
+			description = strings.ReplaceAll(description, endTag, "")
 		}
 	}
 

hack/tools/crd-generator/main_test.go

@@ -312,8 +312,12 @@ func TestOpconTweaksMapRequiredList(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			_, required := opconTweaksMap(tt.channel, tt.props, tt.existingRequired)
-			require.ElementsMatch(t, tt.expectedRequired, required)
+			parentSchema := &apiextensionsv1.JSONSchemaProps{
+				Properties: tt.props,
+				Required:   tt.existingRequired,
+			}
+			opconTweaksMap(tt.channel, parentSchema)
+			require.ElementsMatch(t, tt.expectedRequired, parentSchema.Required)
 		})
 	}
 }