-
Notifications
You must be signed in to change notification settings - Fork 743
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
Update to admissionregistration.k8s.io/v1 #1250
Conversation
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
- clientConfig: | ||
caBundle: Cg== | ||
- admissionReviewVersions: | ||
- v1 |
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.
Are there any differences between the two admission review versions?
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.
it's detailed here: https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG/CHANGELOG-1.16.md#api-changes
doesn't sounds like any major differences, just required fields and api server support for receiving corresponding versions
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
@@ -24,8 +26,11 @@ webhooks: | |||
- UPDATE | |||
resources: | |||
- namespaces | |||
- clientConfig: | |||
caBundle: Cg== |
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.
Looks like caBundle
is removed here but they are persisted in the MutatingWebhookConfiguration
in config/overlays/mutation_webhook/webhook.yaml
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 think config/overlays/mutation_webhook/webhook.yaml
is temporary location, and it's not generated. Once it's generated, it'll go to config/webhook/manifests.yaml
(and caBundle doesn't get generated)
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.
ah ok. should we manually update config/overlays/mutation_webhook/webhook.yaml
for 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.
I don't think it matters but, I updated to be same as it would have been generated.
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.
Related bug: #817
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.
Thanks for linking the issue @maxsmythe so this PR would fix that issue as a bonus.
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
Codecov Report
@@ Coverage Diff @@
## master #1250 +/- ##
==========================================
- Coverage 48.55% 48.49% -0.07%
==========================================
Files 68 68
Lines 4879 4879
==========================================
- Hits 2369 2366 -3
- Misses 2159 2161 +2
- Partials 351 352 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
cert-controller PR: open-policy-agent/cert-controller#34 |
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
still LGTM, if needed |
Though the tests look unhappy |
@maxsmythe failures are due to open-policy-agent/cert-controller#31 that includes controller-runtime v0.8 which includes breaking changes |
Major breaking changes? Or do we just need to tweak some things? |
Sounds like conversion function changes in apimachinery. I think this will have to wait until frameworks repo updates unless we want to revert controller-runtime changes in cert-controller
|
We should explicitly set
I would have linked to a specific file as an example inline but I don't seem to have permission to do that? |
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
@@ -357,14 +368,15 @@ spec: | |||
created: | |||
type: boolean | |||
type: object | |||
version: v1beta1 | |||
type: object | |||
version: v1alpha1 |
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.
@maxsmythe @julianKatz do we want version
to be v1alpha1
or v1beta1
?
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.
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.
version
needs to be equal to the first item in the versions
list. It looks like it doesn't actually do anything except break if it doesn't match:
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.
Note: In apiextensions.k8s.io/v1beta1, there was a version field instead of versions. The version field is deprecated and optional, but if it is not empty, it must match the first item in the versions field.
Looks like version
is deprecated. Do we want to keep it?
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.
It's okay to remove but @julianKatz wasn't able to find a way for controller-gen to generate without version
I believe
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.
Yeah, it should go away when we switch to generating v1 CRDs (which don't have this field)
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
@willbeason thanks! explicitly defined @ritazh @maxsmythe thoughts on whether |
+1 Setting |
|
rebased with @shomron's changes to controller-runtime. @ritazh @maxsmythe still lgty? |
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.
still LGTM!
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
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
Signed-off-by: Sertac Ozercan sozercan@gmail.com
What this PR does / why we need it:
admissionregistration.k8s.io/v1beta1
tov1
. This will change minimum required Kubernetes version to v1.16.controller-gen
tov0.5.0
.Which issue(s) this PR fixes (optional, using
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when the PR gets merged):Fixes #818
Special notes for your reviewer:
admissionregistration.k8s.io/v1beta1
can be upgraded tov1
.