-
Notifications
You must be signed in to change notification settings - Fork 17
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
add ECSNodeClass CEL validation #60
base: main
Are you sure you want to change the base?
Conversation
Thanks to your contribution, the maintainers will review it as soon as they can! |
459f40e
to
97c52db
Compare
pkg/apis/apis.go
Outdated
) | ||
|
||
var ( | ||
Group = "karpenter.k8s.alicloud" | ||
CompatibilityGroup = "compatibility." + Group | ||
CRDs = apis.CRDs // object.Unmarshal[apiextensionsv1.CustomResourceDefinition](crds.ECSNodeClassCRD) | ||
CRDs = append(apis.CRDs, object.Unmarshal[apiextensionsv1.CustomResourceDefinition](config.ECSNodeClassCRD)) |
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.
cc @helen-frank , I remeber we decide to not use this, do you remeber the reason?
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.
The test work needed this, but we didn't have time to write test cases before
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.
Got
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.
This is used to check the validation rule, I think we can launch a kind to and apply the CRD to check whether it's validate.
So, check it in the ut is a little bit redundant and needs to write much code.
What do you think? @daimaxiaxie
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.
If e2e, you need to use a lot of CR yaml to verify the rules. I think it is not as convenient as code.
BTW, what means CEL |
Common Expression Language https://kubernetes.io/docs/reference/using-api/cel/ |
Hi, @daimaxiaxie Thanks! |
it's cool! |
97c52db
to
38432b1
Compare
What type of PR is this?
/kind feature
What this PR does / why we need it:
Added ECSNodeClass CEL validation.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?