SPLAT-1452: Added New FeatureGate/FeatureSet Aware MaxItems #19
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@vr4manta: This pull request references SPLAT-1452 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.16.0" version, but no target version was set. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
/lgtm |
|
@rvanderp3: changing LGTM is restricted to collaborators In response to this:
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/test-infra repository. |
|
/assign @JoelSpeed |
|
/assign @deads2k |
|
@vr4manta: This pull request references SPLAT-1452 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.16.0" version, but no target version was set. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
| } | ||
|
|
||
| if schema.Type != "array" { | ||
| return fmt.Errorf("must apply maxitem to an array") |
There was a problem hiding this comment.
Is it maxitem cased in other error messages?
There was a problem hiding this comment.
I copied this logic from here:
They are using the all lowercase for their output. I can change to maxItems to match the actual field name.
Examples of FeatureGateAwere and normal:
Normal:
// +kubebuilder:validation:MaxItems=20
FG Aware
// +openshift:validation:FeatureGateAwareMaxItems:featureGate=VSphereMultiVCenters,featureGateEnabled=false,maxItems=1
// +openshift:validation:FeatureGateAwareMaxItems:featureGate=VSphereMultiVCenters,featureGateEnabled=true,maxItems=3
The base has var start w/ capital, but feature gate aware starts with lower and upper depending. It seems its not case sensitive.
Example:
// +openshift:validation:FeatureGateAwareEnum:featureGate="",enum="";None;IntegratedOAuth
or
// +openshift:validation:FeatureGateAwareXValidation:featureGate=GCPLabelsTags,rule="!has(oldSelf.resourceLabels) && !has(self.resourceLabels) || has(oldSelf.resourceLabels) && has(self.resourceLabels)",message="resourceLabels may only be configured during installation"
// +openshift:validation:FeatureGateAwareXValidation:featureGate=GCPLabelsTags,rule="!has(oldSelf.resourceTags) && !has(self.resourceTags) || has(oldSelf.resourceTags) && has(self.resourceTags)",message="resourceTags may only be configured during installation"
There was a problem hiding this comment.
If you look at how the enum version of this works, the default case is with an empty feature gate, can we copy the same pattern so there's consistency?
I'd also like an example usage of this marker added to the same example APi once this is merged please
There was a problem hiding this comment.
So problem with default when I tested it was that it was also firing casuing my value of 3 to get overridden by the 1. Thats why I added the second flag. I can retest this , but I am pretty sure this is what I saw resulting in me adding this new variable.
For the example api, I can add it there. I haven't seen the example before so I will ping you offline about this. Thanks!
There was a problem hiding this comment.
Based on our discussion, I've made the changes to follow enum's example. I've confirmed this is looking great. I have pushed the changes.
For the api example, I'll make that change and push w/ my PR at top of this PR (marked as blocker).
pkg/crd/markers/patch_validation.go
Outdated
| @@ -143,6 +149,25 @@ func (m FeatureGateEnum) ApplyToSchema(schema *apiext.JSONSchemaProps) error { | |||
| return nil | |||
| } | |||
|
|
|||
| type FeatureGateMaxItems struct { | |||
| FeatureGateNames []string `marker:"featureGate"` | |||
| FeatureGateEnabled bool `marker:"featureGateEnabled"` | |||
There was a problem hiding this comment.
What's this looking at? Can you provide an example of using this?
I guess this is so you can have one version with the feature gate and one without? Is this how it works for other feature gate aware markers?
There was a problem hiding this comment.
The featureGateEnabled was a test i tried. I can remove that one to match the others. For some reason I thought I had copied that from another existing one, but now I am not sure.
There was a problem hiding this comment.
Oh I remember now, I needed a way to say if my feature gate is disabled, to do something. See example in comment about syntax above. Also, the PR from blocker section in PR description has usage and outputs to show this logic working.
|
@vr4manta: This pull request references SPLAT-1452 which is a valid jira issue. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
Spoken with @vr4manta, this now is in keeping with the enum version for usage, an example will be added to the o/api examples as well |
SPLAT-1452
Changes
Blocks
Use Case
This enhancement is being done for the vSphere Multi vCenter feature. The idea is we want to have max vCenter count == 3 initially for Tech Preview. By adding maxItemsAware we will have a way to set Default stay at 1, but for TP / Custom, allow vCenter count to be a max of 3. Also by setting this way with the maxItemsAware, we will be able to have the CRD limit it so all operators do not have to be max count aware (check for 3 vs just checking for feature enabled).
Example of new validation field: