resolving KEP comments round kubernetes#6

Author: aaron-prindle

keps/sig-api-machinery/5073-declarative-validation-with-validation-gen/README.md

@@ -868,15 +868,15 @@ In Kubernetes there are internal schema representations and versioned schema rep
 
 Declarative validation has challenges dealing with zero values in Go types. The core issue stems from the fact that zero values can be treated both as valid inputs and equivalent to unspecified or unset values.. This creates discrepancies between how Go code handles validation and how declarative validation, based on the schema, would interpret the same data.  Ex: `ReplicationControllerSpec.MinReadySeconds` might legitimately be set to `0`, indicating that a pod is considered available immediately. This challenges the general assumption in some contexts that zero values for optional fields are inherently invalid, as Kubernetes can treat in some cases as set values or defaults.
 
-#### Difficulties with `+required` and `+default`
+#### Difficulties with `+k8s:required` and `+k8s:default`
 
-The straightforward approach of using the `+required` tag to enforce the presence of a field fails when the zero value is valid. Applying `+required` can incorrectly reject legitimate zero values. Similarly, using `+default` to explicitly document the default value (even if it's the zero value) creates problems because `+default` implies requiredness on the server side.
+The straightforward approach of using the `+k8s:required` tag to enforce the presence of a field fails when the zero value is valid. Applying `+k8s:required` can incorrectly reject legitimate zero values. Similarly, using `+k8s:default` to explicitly document the default value (even if it's the zero value) creates problems because `+k8s:default` implies requiredness on the server side.
 
 #### Proposed Solutions
 
-1. <strong>Tri-State mutually exclusive options (Recommended): `+optional`, `+default`, `+required`:</strong>
-    *   Treat `+optional`, `+default`, and `+required` as mutually exclusive options.
-    *   Fields that allow valid zero values and have defaults would be explicitly tagged with neither `+optional` nor `+required`.
+1. <strong>Tri-State mutually exclusive options (Recommended): `+k8s:optional`, `+k8s:default`, `+k8s:required`:</strong>
+    *   Treat `+k8s:optional`, `+k8s:default`, and `+k8s:required` as mutually exclusive options.
+    *   Fields that allow valid zero values and have defaults would be explicitly tagged with neither `+k8s:optional` nor `+k8s:required`.
     *   Validation logic would need to be aware of this and handle zero values appropriately for such fields.
     *   <strong>Drawback:</strong> This approach requires a linter to enforce the tri-state rule and prevent invalid combinations.
     *   <strong>Benefit:</strong> Simplifies the mental model by making the relationship between optionality, defaults, and requiredness explicit.
@@ -1033,26 +1033,18 @@ func TestVersionedValidationByFuzzing(t *testing.T) {
 ```
 
 ###### strategy_test.go vs validation_test.go
-<<[UNRESOLVED should we move current validation_test.go logic to strategy_test.go as it makes more sense generally and aids this project? ]>>
-
-
 Currently, validation logic and associated tests are logically split across `validation.go` and `strategy.go`. The hand-written validation functions and associated tests reside in `validation.go` and `validation_test.go`  while `strategy.go` determines when to invoke these functions during API object creation and updates. With the introduction of declarative validation (controlled by the `DeclarativeValidation` feature gate)  the current logic split is worth considering moving from `validation_test.go` to `strategy_test.go` as the current logic split in the test is unfavorable for the migration as it requires additional plumbing work.
 
-The current approach in the prototype experiment to migrate ReplicationController involves directly injecting declarative validation and equivalency tests into `validation_test.go`. This is achieved by conditionally appending calls to `rest.ValidateDeclaratively` within the existing test cases, based on the `DeclarativeValidation` feature gate's status. This allows for a direct comparison of the outputs between hand-written and declarative validation within the same test framework.
-
-**Moving the feature gate check and declarative validation logic to `strategy_test.go` could offer several advantages</strong>:
+The current approach in the prototype experiment to migrate ReplicationController involves directly injecting declarative validation and equivalency tests into `validation_test.go`. This is achieved by conditionally appending calls to `rest.ValidateDeclaratively` within the existing test cases, based on the `DeclarativeValidation` feature gate's status. This allows for a direct comparison of the outputs between hand-written and declarative validation within the same test framework.  For v1.33 we plan on using this method for the initial small migration where we land the core pieces of `validation-gen`
 
+For v1.34 we plan on  - **moving the feature gate check and declarative validation logic to `strategy_test.go`**.  Doing this has the following benefits:
 *   **Reduced Test Duplication:** `validation_test.go` could be simplified, as it would no longer need to handle both hand-written and declarative validation paths.
 *   **Clearer Separation of Concerns:** `strategy.go` would be responsible for determining _when_ to validate, while `validation.go` would handle _how_ to validate.
 *   **Easier Migration:**  Transitioning to a fully declarative model would be smoother, as the core validation logic would already be invoked through the strategy.
 
-However, this approach also has potential drawbacks:
+Doing this requires an additional PR to moving existing hand-written validation logic from `validation.go` to `strategy.go` would but it would be straightforward (only moving files). This would be done **in a PR after the initial migration PR in v1.34 but before any additional migration work is done.**
 
-*   **Additional PRs for Initial Migration:** Moving existing hand-written validation logic from `validation.go` to `strategy.go` would result in additional work via a PR to move the related code but it would be straightforward (only moving files).
-
-Given the low complexity of moving this code prior to the changes, the enhanced logic split of moving the code, and the reduced work for the migration that moving this code would have currently the plan for Declarative Validation is that the current `validation_test.go` tests are moved to `strategy_test.go` in a PR prior to the migration PR.  
-
-<<[/UNRESOLVED]>>
+Given the low complexity of moving this code prior to the changes, the enhanced logic split of moving the code, and the reduced work for the migration that moving this code would have currently the plan for Declarative Validation is that the current `validation_test.go` tests are moved to `strategy_test.go`.
 
 ###### Error Message Equivalence