🐛 Fix int or string pattern application #611
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
@JoelSpeed can we properly test this by extending pkg/crd/testdata/cronjob_types.go with a field of type IntOrString and a pattern? |
Yes of course! I did have a quick scan for something testing the CRD generation but didn't find this, will take a look at adding a test today |
| to a perecentage or integer. The XIntOrString marker is a requirement | ||
| for having a pattern on this type. | ||
| pattern: ^((100|[0-9]{1,2})%|[0-9]+)$ | ||
| x-kubernetes-int-or-string: true |
There was a problem hiding this comment.
Did you btw if the apiserver correctly deals with this (i.E. applies the pattern when you submit a string but not when you submit an int)?
There was a problem hiding this comment.
Yeah I tested this manually in kind. I could apply any number I wanted but was restricted if I added a percent sign
k8s-ci-robot
added
the
tide/merge-method-squash
k8s-ci-robot
added
the
lgtm
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman, JoelSpeed, vincepri The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
This allows users to set the schema.XIntOrString property during the marker validation phase. Which in turn allows other markers to rely on the fact that the type is an IntOrString. Eg. This allows you to set a pattern on an IntOrStr type
JoelSpeed
force-pushed
the
fix-int-or-string-pattern
branch
from
1dc1a21
to
f1dc0d0
Compare
k8s-ci-robot
removed
the
lgtm
|
Had to fix a typo in the comment on the new marker, no other changes |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
This is a follow up to fix #608 which I hadn't appropriately tested before proposing it 😞
I tried this out this morning and worked out that it wasn't working, there are two reasons for this.
The condition check was wrong, we meant to check for the schema not being an IntOrString.
The second issue is that the additional information for the type that the generator applies (eg setting XIntOrString=true) is not done until way later in the generation than the markers being applied. Looking at the existing code, the "fix" for this seems to be that you have to apply an additional marker, eg the Type marker, that sets the required fields early.
So this introduces a new marker
// +kubebuilder:validation:XIntOrStringwhich marks the type as an IntOrString early to allow the pattern marker to pick this up. Not sure if this is the best approach given Solly's old notes, but it seems to be in-keeping with the rest of the markers/CC @alvaroaleman @vincepri