Skip to content
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

Move validation to be part of the CRD #3321

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mercybassey
Copy link

Description:

Link to tracking Issue(s):

Testing:

Documentation:

@mercybassey mercybassey requested a review from a team as a code owner October 3, 2024 19:12
@mercybassey
Copy link
Author

@swiatekm Based on my changes, I just wanted to know if am on the right track before I proceed for all CRDs. Thanks.

Copy link
Contributor

@swiatekm swiatekm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're making changes in the right place, but I'm not sure they're the right changes for the effect you want.

Try running make bundle and examining the result of your changes to the generated CRDS. This should give you a better sense of whether you're on the right path.

@@ -90,10 +90,10 @@ type OpenTelemetryCollectorSpec struct {
// +optional
TargetAllocator TargetAllocatorEmbedded `json:"targetAllocator,omitempty"`
// Mode represents how the collector should be deployed (deployment, daemonset, statefulset or sidecar)
// +optional
// +kubebuilder:validation:Enum=ModeDeployment;ModeDaemonSet;ModeStatefulSet;ModeSidecar
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This specific change is superfluous. If you look at the generated CRD, this validation is already there. If we look at what's missing compared to the webhook, we should add a marker here to set the default to deployment.

Keep in mind that the markers deal with raw values of the field as they appear in the resource in K8s, not the aliases we have for them in Go code. So you should use deployment instead of ModeDeployment.

@mercybassey
Copy link
Author

@swiatekm I addressed your comment and added more validation rules. I'll appreciate your review. Thank you.

@@ -110,6 +113,7 @@ type OpenTelemetryCollectorSpec struct {
// functionality is only available if one of the valid modes is set.
// Valid modes are: deployment, daemonset and statefulset.
// +optional
// +kubebuilder:validation:Enum=deployment;daemonSet;statefulSet;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not correct

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a mistake. I'll remove the extra ;.

@mercybassey
Copy link
Author

@pavolloffay I have fixed it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move validation to be part of the CRD
3 participants