🌱 new tag symmetry and required validations #2358
Conversation
Signed-off-by: grokspawn <jordan@nimblewidget.com>
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
This PR adds symmetric support for standard-specific field descriptions (complementing the existing experimental-specific descriptions) and introduces optional/required validation tags that allow field annotations to dynamically control the required fields list in generated CRDs. The changes maintain backward compatibility while extending the CRD generation capabilities.
Key Changes
- Added support for
<opcon:standard:description>tags (symmetric to existing experimental tags) - Implemented
<opcon:*:validation:Optional>and<opcon:*:validation:Required>tags to control field requirement status - Enhanced
opconTweaksandopconTweaksMapfunctions to return and process requirement status, updating the required fields list accordingly - Added comprehensive unit tests covering all new tag functionality including edge cases
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| hack/tools/crd-generator/main.go | Extended opconTweaks and opconTweaksMap to handle required status; refactored formatDescription to support both standard and experimental description tags symmetrically; added Optional/Required validation tag processing |
| hack/tools/crd-generator/main_test.go | Added comprehensive test suites for formatDescription, opconTweaks optional/required handling, and opconTweaksMap required list manipulation |
| hack/tools/crd-generator/README.md | Documented new Optional and Required validation tags, and added Standard Description section to mirror Experimental Description documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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 | ||
| } | ||
| } |
There was a problem hiding this comment.
[nitpick] If a field description contains multiple Optional/Required tags (e.g., both <opcon:standard:validation:Optional> and <opcon:standard:validation:Required>), the last one processed will determine the final status. Consider adding validation to detect and error on conflicting tags, or document that only one such tag should be used per field.
There was a problem hiding this comment.
Yeah, we should error if we try to set this a second time.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2358 +/- ##
==========================================
- Coverage 74.23% 70.63% -3.61%
==========================================
Files 91 91
Lines 7239 7266 +27
==========================================
- Hits 5374 5132 -242
- Misses 1433 1699 +266
- Partials 432 435 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: grokspawn <jordan@nimblewidget.com>
What is the motivation for adding this logic, if there are not need for it? Should we postpone it and add within a PR where such logic is needed? |
|
@pedjak motivation is making the |
| // 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 | ||
| } |
There was a problem hiding this comment.
I was originally thinking that we'd pass parentSchema all the way down into opconTweaks (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 opconTweaks to make any/all necessary changes to the parent to account for the markers that are on the name-ed field?
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: joelanford The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
Thanks @joelanford for the context. I see now that is extracted from #2355 |
| // the limitations of Kubebuilder annotations. | ||
| func opconTweaks(channel string, name string, jsonProps apiextensionsv1.JSONSchemaProps) *apiextensionsv1.JSONSchemaProps { | ||
| // 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). |
There was a problem hiding this comment.
instead of strings, would it make sense to use a *bool that can represent these three states as well?
There was a problem hiding this comment.
bool has a limited range of values (even if you include nil with *bool). Using string allows for additional flexibility.
| // 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). |
There was a problem hiding this comment.
this is the documentatino for opconTweaks function? Should we move it next to the function signature?
4 participants
Description
Adds new standard:description tag support to allow inclusion of standard-manifest-only field descriptions.
Adds new optional/required validation tag support to allow field tags to impact the required list of their parent field.
Adds unit tests for all new capabilities, since there are no existing uses of the new tags (yet!).
Reviewer Checklist