-
Notifications
You must be signed in to change notification settings - Fork 7
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
GH-110 use CRD validation on DNSRecord #160
Conversation
5b68ace
to
f289a0b
Compare
Port *int `json:"port,omitempty"` | ||
|
||
// +kubebuilder:validation:XValidation:rule="self in ['HTTP','HTTPS']",message="Only HTTP or HTTPS protocols are allowed" | ||
Protocol *HealthProtocol `json:"protocol,omitempty"` |
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.
Using Enum would probably make more sense here, and for port see https://github.com/Kuadrant/kuadrant-operator/blob/main/api/v1alpha1/dnspolicy_types.go#L255
Also, all the validations should be the same as https://github.com/Kuadrant/kuadrant-operator/blob/main/api/v1alpha1/dnspolicy_types.go#L246-L261
@philbrookes Why is that duplicated? Would it not make more sense for the DNSPolicy to reference this HealthCheckSpec type?
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.
I cannot remember clearly, I suspect the Specs were different from each other at some point, but are aligned now.
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.
as the validations in this PR are superior to the validations in Kuadrant-operator, it might be worth a PR to update kuadrant to use 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.
I'd prefer the CEL validations - way nicer error message if things aren't right.
Hmm, would it be a good idea to remove duplication as part of this issue?
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.
Opened a PR to the kuadrant-operator as well
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.
Some small changes to the language used, the regexes look fine.
I see @mikenairn has added some tests to cover his validations, which is pretty neat, is that something we could also do here? |
Added test @philbrookes. It is a strange one, but, hopefully, is useful. At least it is cheep to check it. |
Adding following validation rules on the DNS Record CRD:
Spec
.
. This is the only check from theValidate()
that is feasible to transfer to the CRD validationsHealthCheck Spec
?
or/
, contain only allowed for the URLs symbols and end with alphanumeric character or/
.HTTP
orHTTPS