-
Notifications
You must be signed in to change notification settings - Fork 690
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
IngressRoute CRD Validation #441
Conversation
4ae63dc
to
81f4e1e
Compare
deployment/common/common.yaml
Outdated
properties: | ||
fqdn: | ||
type: string | ||
pattern: ^([a-z0-9]+(-[a-z0-9]+)*\.)+[a-z]{2,}$ |
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.
Are uppercase chars not allowed in FQDNs?
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.
Yup they are allowed, just was going with the default that they are typically lowercase. Will update to follow the spec.
deployment/common/common.yaml
Outdated
type: string | ||
pattern: ^\/.*$ | ||
delegate: | ||
type: string |
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.
Should this be type: object
?
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 should be object
deployment/common/common.yaml
Outdated
required: | ||
- name | ||
- namespace | ||
pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$ # DNS-1123 |
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.
extra/left-over pattern?
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, similar to previous, will remove
@alexbrand @stevesloka you two know the most about defining validations. Please merge this when you are happy. Thanks Dave |
Signed-off-by: Steve Sloka <[email protected]>
Signed-off-by: Steve Sloka <[email protected]>
c154337
to
ac07c5a
Compare
@alexbrand PR comments should all be resolved now |
Signed-off-by: Steve Sloka <[email protected]>
@stevesloka can you please rerun |
Signed-off-by: Sunjay Bhatia <[email protected]>
This adds CRD Validation to the IngressRoute spec. One thing to note is it defines an Enum of values allowed for the
strategy
field which is not yet implemented. We may need to return to this validation once that feature lands.// Fixes #416
Signed-off-by: Steve Sloka [email protected]