-
Notifications
You must be signed in to change notification settings - Fork 612
Use CRD & Webhook v1 APIs #3360
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
Conversation
@dprotaso FYI I've started this on eventing |
This change is required for conversion with v1 CRD APIs to work |
@dprotaso thanks for the heads-up, I've updated the PR |
- name: v1beta1 | ||
served: true | ||
storage: true | ||
# conversionReviewVersions: ["v1beta1", "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.
scope: Namespaced | ||
conversion: | ||
strategy: Webhook | ||
conversionReviewVersions: ["v1beta1", "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.
These versions are the payload being set to the webhook - not the CRD version being reviewed
conversionReviewVersions: ["v1beta1", "v1alpha1"] | |
conversionReviewVersions: ["v1", "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.
We need both v1 and v1beta1 because our min K8s version is 1.16.
The API imposes a restriction that we support conversionReview version that are available on K8s 1.15 & 1.16.
We can't drop v1beta1 it was the only available version in 1.15
a94d511
to
d012f2a
Compare
/hold |
# see issue: https://github.com/knative/serving/issues/912 | ||
x-kubernetes-preserve-unknown-fields: true | ||
versions: | ||
- &version |
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.
well, we do that already with everything
on the verbs ;-)
but yeah 🙄
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.
Nah, it is fine :) Just some YAML magic that I didn't know before :)
Actually gets rid of some dupes, which is nice
/retest |
/hold waiting for review from at least @dprotaso |
/retest |
does this PR also updates the MutatingWebhookConfiguration and ValidatingWebhookConfiguration CRs to v1? |
no - we should track bumping this as well, since its relevant for all of knative, at some point... This PR is a follow up of the work that @dprotaso did on |
/test pull-knative-eventing-integration-tests |
1 similar comment
/test pull-knative-eventing-integration-tests |
migrator job is failing
I'd recommend dumping some logs in the test for more visibility |
clientConfig: | ||
service: | ||
name: eventing-webhook | ||
namespace: knative-eventing |
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 you can't drop the v1alpha1 for the brokers based on this from the failed upgrade tests:
for: "STDIN": CustomResourceDefinition.apiextensions.k8s.io "brokers.eventing.knative.dev" is invalid: status.storedVersions[0]: Invalid value: "v1alpha1": must appear in spec.versions
Error from server (Invalid): error when applying patch:
Or:
for: "STDIN": CustomResourceDefinition.apiextensions.k8s.io "pingsources.sources.knative.dev" is invalid: status.storedVersions[0]: Invalid value: "v1alpha1": must appear in spec.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.
Yeah this requires the storage migration job to be run before the upgrade
I do this after the upgrade in serving which is technically wrong (we've never dropped a version yet so we didn't catch it)
/test pull-knative-eventing-upgrade-tests |
ecb9ced
to
b69d793
Compare
/unhold |
The following jobs failed:
Failed non-flaky tests preventing automatic retry of pull-knative-eventing-upgrade-tests:
|
Signed-off-by: Matthias Wessendorf <[email protected]>
lgtm letting @vaikas do the heavy work |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: matzew, vaikas The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Matthias Wessendorf [email protected]
Fixes #3349
Proposed Changes
v1
of the CRD typeRelease Note
Docs