-
Notifications
You must be signed in to change notification settings - Fork 828
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
refactor(crds): Use built-in OpenAPI validation #5129
Conversation
2254382
to
6ca6900
Compare
@@ -25,42 +25,77 @@ import ( | |||
"github.com/seldonio/seldon-core/apis/go/v2/mlops/scheduler" | |||
) | |||
|
|||
//+kubebuilder:object:root=true |
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.
explain: More conventional kubebuilder annotations to have this for both struct
s, and to be ordered this way. This is also the same for the new lines between fields.
// Synchronous output from this pipeline, optional | ||
Output *PipelineOutput `json:"output,omitempty"` | ||
} | ||
|
||
// +kubebuilder:validation:Enum=inner;outer;any | ||
type JoinType string | ||
|
||
const ( |
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.
explain: It's not ideal that it's repeated here, but this approach seems common [1].
[1] https://github.com/search?q=%2Bkubebuilder%3Avalidation%3AEnum&type=code — e.g. Grafana, Cilium, etc.
2021065
to
8fd7e0e
Compare
Part of a progressive change to use the CRD validation that's available via kubebuilder [1]. This has the benefits of us: - Not having to write the code for the validation, since it's generated/handled via OpenAPI - It's structurally/programatically available in the spec - Less repetition [1] https://book.kubebuilder.io/reference/markers/crd-validation.html
8fd7e0e
to
be4560c
Compare
RemainingItemCount: nil, | ||
}, | ||
Status: "Failure", | ||
Message: fmt.Sprintf("Pipeline.mlops.seldon.io \"%s\" is invalid: [metadata.name: Invalid value: \"%s\": must be no more than 253 characters, metadata.name: Invalid value: \"%s\": a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*'), spec.steps: Required value]", pipelineName, pipelineName, pipelineName), |
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.
* refactor(crds): Use built-in OpenAPI validation Part of a progressive change to use the CRD validation that's available via kubebuilder [1]. This has the benefits of us: - Not having to write the code for the validation, since it's generated/handled via OpenAPI - It's structurally/programatically available in the spec - Less repetition [1] https://book.kubebuilder.io/reference/markers/crd-validation.html
Part of a progressive change to use the CRD validation that's available via kubebuilder [1].
This has the benefits of us:
This uses the generate
suite_test.go
for now [2].[1] https://book.kubebuilder.io/reference/markers/crd-validation.html
[2] https://book.kubebuilder.io/cronjob-tutorial/writing-tests