-
Notifications
You must be signed in to change notification settings - Fork 53
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
chore: updating to use 0.30.3 k8s client #461
Conversation
39ba590
to
69e004d
Compare
@@ -84,7 +84,7 @@ func (d *Driver) AddTemplate(_ context.Context, ct *templates.ConstraintTemplate | |||
return err | |||
} | |||
vapVars = append(vapVars, vapVarsSuffix...) | |||
filterCompiler, err := cel.NewCompositedCompiler(environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion())) | |||
filterCompiler, err := cel.NewCompositedCompiler(environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion(), false)) |
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.
why do we want to default to false here? can you add a comment
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.
In k8s code I see places that defaults it to true?
// strictCost is always true to enforce cost limits.
celEnvironmentSet: environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion(), 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.
false
here means that we are not enforcing strictCost
calculation for CEL, which was the default behavior before this change was introduced.
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.
We should adopt whatever K8s is doing, since this is the release that will make our engine beta... we can always loosen the cost calculations, tightening them may break backwards compatibility.
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.
updated to follow K8s.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #461 +/- ##
==========================================
- Coverage 54.68% 53.80% -0.88%
==========================================
Files 71 104 +33
Lines 5241 6746 +1505
==========================================
+ Hits 2866 3630 +764
- Misses 2073 2745 +672
- Partials 302 371 +69
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
LGTM once costing discussion is resolved.
We should merge #435 first |
We also need another PR to conditionally support admissionregistrationv1beta1 and admissionregistrationv1 |
This we can handle on GK side based on the available APIVersion, here |
Signed-off-by: Jaydip Gabani <[email protected]>
Signed-off-by: Jaydip Gabani <[email protected]>
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.
LGTM pending @ritazh and @maxsmythe's comments
Signed-off-by: Jaydip Gabani <[email protected]>
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.
LGTM
I see r.scheme.Convert is added in GK to convert between these versions before writing/reading to/from the api server. |
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.
LGTM
No description provided.