-
Notifications
You must be signed in to change notification settings - Fork 5.3k
docs: add Declarative Validation Native (DV Native) to api-conventions.md and api_changes.md #8741
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
Open
aaron-prindle
wants to merge
1
commit into
kubernetes:master
Choose a base branch
from
aaron-prindle:dv-native-docs
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+46
−0
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
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.
it's a little unfortunate to have to add this to all DV-only fields for the future ... is there not a way to indicate this only in the strategy / tests and not need need to pollute the field docs with this?
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.
/cc @lalitc375
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.
Yes, it is possible to indicate this in the strategy.go files. This will tie our hands, forcing us to use only stable validations tags for that kind. This means we will not be able to use alpha/beta tags mirrored with handwritten validation code in that kind.
Secondarily, Eventually we don't want to restrict DV native for new API's. We want to also enable DV native validations for new fields added to existing kinds. This gives us flexibility to do that.
We hear your feedback. If a Kind only has all stable validations. They we just need to put this new tag on one super field i.e., spec. This will automatically mark all validations of the spec field and its subfields as native. This should not pollute the Kind that much.
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.
Does the DV test framework prevent adding a DV validation without a covering test? If not, that seems like a good gap to close.
Are DV validation tests required to exercise in DV and non-DV mode OR explicitly indicate the validation it is testing is DV-native? If not, that seems like a good gap to close. If so, wouldn't we run DV-native tests with only stable DV validations enabled? That seems like it would cover what we need without requiring native-indication in the field docs.
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.
There are no tests to test this. This responsibility is on the reviewers right now. There is an AI prompt to make sure all DV tags have tests. https://github.com/gke-labs/gemini-for-kubernetes-development/blob/main/commands/dv/review.toml
Yes, DV validation tests required to exercise in DV and non-DV mode. We segregate native validations errors from all dv errors, there is a bool present in the error struct to indicate whether it is from a native validation or not. Test cases need to explicitly mention which type of validations errors they are expecting for a given failure scenario. Mismatches in this will result an test failure and leaks will be caught.
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.
That might be a good improvement to add to the backlog. If adding a DV tag couldn't merge until a test that exercised it was added, that would be ideal.
So... can we scope the DV-native indicator to the test definition that is exercising the specific test? That would skip running the non-DV version, and would run the DV version with only stable tags enabled, and make sure the tested validation still works? That seems like it would give similar granularity without needing to pollute the type docs