wip: feat(requiredfields): add K8s Declarative Validation marker support#228
wip: feat(requiredfields): add K8s Declarative Validation marker support#228sivchari wants to merge 1 commit intokubernetes-sigs:mainfrom
Conversation
k8s-ci-robot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sivchari The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
approved
| markers.KubebuilderMinLengthMarker, | ||
| markers.K8sMinLengthMarker, | ||
|
|
||
| // MinProperties markers |
There was a problem hiding this comment.
@everettraven do you know if there's anything in progress for MinProperties in DV?
There was a problem hiding this comment.
Not that I'm aware of. Issue here: jpbetz/validation-gen#251
There was a problem hiding this comment.
Since DV doesn't have minProperties yet (jpbetz/validation-gen#251), only the kubebuilder marker is registered here. I'm happy to add k8s:minProperties support if needed.
|
|
||
| // Check +k8s:required marker (for declarative validation) | ||
| // If +k8s:required is present but neither +required nor +kubebuilder:validation:Required is present, suggest adding the preferred one | ||
| hasPreferredRequired := fieldMarkers.Has(markers.RequiredMarker) |
There was a problem hiding this comment.
Shouldn't this be based on the configured preferred required marker?
|
|
||
| // checkK8sRequired checks for +k8s:required marker usage. | ||
| // +k8s:required is for declarative validation and is separate from +required or +kubebuilder:validation:Required markers. | ||
| // If the field has +k8s:required but doesn't have +required or +kubebuilder:validation:Required, we suggest adding the preferred one. |
There was a problem hiding this comment.
At some point, we expect that k8s:required will be the preferred singular choice. So maybe we need to be able to configure the preferred marker also as k8s:required, and in that case, not suggest adding anything additional
pkg/analysis/utils/zero_value.go
Outdated
| return fieldMarkers.Has(markers.KubebuilderEnumMarker) | ||
| // Check if the field has a kubebuilder enum marker on the field, | ||
| // or if the field's type has a +k8s:enum marker. | ||
| // Note: fieldMarkers includes type markers via TypeAwareMarkerCollectionForField. |
There was a problem hiding this comment.
What happens if the +k8s:enum is on the field, not the type? We probably don't want to allow that?
Add support for K8s DV markers in the requiredfields linter: - +k8s:required marker detection with +required suggestion - +k8s:minLength, +k8s:minimum, +k8s:maximum, +k8s:minItems markers - +k8s:enum support with const value analysis to detect if empty string is a valid enum value The k8sEnumAllowsEmpty function analyzes const values of enum types using go/types package to determine if empty string is allowed. Signed-off-by: sivchari <shibuuuu5@gmail.com>
sivchari
force-pushed
the
feat/requiredfields-k8s-dv-markers
branch
from
0798af6 to
24d1ae2
Compare
k8s-ci-robot
added
size/XL
4 participants
Add support for K8s DV markers in the requiredfields linter:
The k8sEnumAllowsEmpty function analyzes const values of enum types using go/types package to determine if empty string is allowed.
Fixes: #224