-
Notifications
You must be signed in to change notification settings - Fork 40.9k
[WIP] [Don't review] Validation gen #132629
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
base: master
Are you sure you want to change the base?
Conversation
This reverts commit 178b781.
Also fix boilerplate errors
This crashes during codegen with a pretty useless panic(), which doesn't make sense any more. ``` $ pushd; ./hack/update-codegen.sh valid; pushd ~/src/kubernetes ~/src/kubernetes/staging/src/k8s.io/code-generator/cmd/validation-gen +++ [0418 23:56:03] validation: 69 targets panic: alias type should already have been unwrapped goroutine 1 [running]: k8s.io/code-generator/cmd/validation-gen/validators.requirednessTagValidator.GetValidations({{0x86cb10?, 0xc0000c0b60?}}, {{0x86ca5a, 0xd}, 0xc01ff9efc0, 0xc01ff9f0a0, 0xc02a3f0cd0, 0xc02a4092c0}, {0x0, 0x0, ...}, ...) /home/thockin/src/kubernetes/staging/src/k8s.io/code-generator/cmd/validation-gen/validators/required.go:69 +0x40f k8s.io/code-generator/cmd/validation-gen/validators.(*registry).ExtractValidations(0xc0a4e0, {{0x86ca5a, 0xd}, 0xc01ff9efc0, 0xc01ff9f0a0, 0xc02a3f0cd0, 0xc02a4092c0}, {0xc0200640e0, 0x2, 0x2}) ``` Subsequent commits will clean this area up more.
Before this, pointer fields would fail at codegen.
Prior to this it would look at the .Kind, see "Alias", and generate calls to (e.g.) `RequiredValue()`. Now calls `RequiredSlice()`.
I did an audit of how we are handling pointerness and aliases in validators. In a word - inconsistent. I went through each validator and added test cases where they were missing (previous commits). This commit changes how Context.Type is defined for typedefs. Previously Context.Type was the potentially aliased and potentially pointerful type for struct fields, but the "real" concrete type for typedefs. Now it is consistent, and validators need to unalias and unpointer types (which they had to do anyway because it was inconsistent.
Previous commits focused on checking for pointers and aliases more appropriately. What emerged was two functions `realType` which gave us the concrete native type (non-pointer, non-alias) an `unaliasType` which stripped aliasing but only until it hit pointerness. They were sort of inconsistently applied and in a few places I found the composition of them to be weird. I want to reframe those as: 1) `func nativeType(t) t` which removes all aliasing but retains pointerness. So for example: ``` type T1 string type T2 *T1 type T3 *T2 type Foo struct { Name *T3 } ``` Calling `nativeType()` on the `Name` member would yield `***string`. This is what we want in most places. We need to retain pointerness because (e.g.) we DO NOT support pointer to slice, and we want that to fail, but we DO support pointer to string and we want that to succeed, but `optional` needs to handle pointers and non-pointers differently. We filter totally disallowed things early on, but if a pointer to slice should sneak in somehow, this will refuse to generate bad code (fail in an obvious way). 2) `func nonPointer(t) t` which removes all pointerness (if present), but does not look past aliases. We needed to compose these both and call `nonPointer(nativeType(t))` in a few places. E.g. when applying `+k8s:format`, we need to know that the field is a string OR pointer to string. These two are more composable, I think. If you agree I can finish that off this weekend and ping the PR. If not, would like to hear your thoughts. Mostly the goal is put as many defensive checks in place as possible. New validators will copy these ones and I want it to be as clear as possible.
…-view chore: update +k8s:neq tests and error text based on review comments
We don't care if the values of the members changed, only which members are set / unset.
…ich_fields_are_set_not_values Change union ratcheting to only look at membership
The custom IP address validation format used within Kubernetes was previously named 'k8s-ip-sloppy'. This name was chosen as a temporary placeholder to reserve the 'k8s-ip' format name for a future, stricter IP validation implementation. With the introduction of validation ratcheting, it is now possible to evolve validation rules for fields without breaking existing stored objects. This makes the "sloppy" suffix redundant, as the validation for 'k8s-ip' can be tightened over time as needed.
refactor(validation): Rename k8s-ip-sloppy format to k8s-ip
…heting No lookup correlated for non-map/set list in ratcheting
This caused some changes in generated code which brought to light the fact that we are not considering list-metadata which is defined on typedefs when emitting code for fields which use those typedefs. I will fix that in a subsequent commit, and these deltas will be corrected.
fix: check error isValid() before render interface
chore: fix forward on validation versioned test
Subsequent changes are more obvious with this done first.
When we have a typedef to a list, and we define list metadata (e.g. listType=map) on the typedef, we were not considering that when we emit uniqueness checks. Additionally, we did not check that the user didn't specify incompatible metadata on both the type and the field. Now we disallow those tags to be in both places, and we look at type-attached metadata if there is not field-attached metadata. We should never have both.
Field validators come last. I think we can depend on that, and it is consistent with main discovery (a field's type before the field itself).
The path is sometimes a type path.
Atomic is the default, but if someone specifies it explicitly, we should consider it for conflicts like set and map.
…efs_and_fields Fix HEAD, fix uniqueness checks for typedefs
42c9b0f
to
b65c3d0
Compare
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: lalitc375 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@lalitc375: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
c268ebb
to
7a4b6be
Compare
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
[WIP] [Don't review] Validation gen - Test PR.