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

Add a job for migrating sinkbindings and containersource storage versions to v1beta1 #3895

Merged
merged 3 commits into from
Aug 25, 2020

Conversation

nlopezgi
Copy link
Contributor

Fixes #3674, #3615

Proposed Changes

Release Note

  • 🗑️ Remove feature or internal logic
    Eventing v1alpha1 API is removed.
    action required: You must run pre-install job prior to upgrading to get sinkbindings and containersource to v1beta1 API.

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Aug 20, 2020
@knative-prow-robot knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 20, 2020
@nlopezgi nlopezgi changed the title add a job for migrating sinkbindings and containersource storage versions to v1beta1 [WIP] add a job for migrating sinkbindings and containersource storage versions to v1beta1 Aug 20, 2020
@knative-prow-robot knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 20, 2020
@matzew
Copy link
Member

matzew commented Aug 20, 2020

/assign @devguyio

@knative-test-reporter-robot

The following jobs failed:

Test name Triggers Retries
pull-knative-eventing-conformance-tests 0/3

Failed non-flaky tests preventing automatic retry of pull-knative-eventing-conformance-tests:

test/conformance.TestChannelStatusSubscriber/Channel_has_required_status_subscriber_fields
test/conformance.TestChannelStatusSubscriber/Channel-messaging.knative.dev/v1beta1
test/conformance.TestChannelStatusSubscriber

@nlopezgi
Copy link
Contributor Author

/test pull-knative-eventing-conformance-tests

@nlopezgi
Copy link
Contributor Author

/test pull-knative-eventing-unit-tests

@nlopezgi nlopezgi changed the title [WIP] add a job for migrating sinkbindings and containersource storage versions to v1beta1 Add a job for migrating sinkbindings and containersource storage versions to v1beta1 Aug 21, 2020
@knative-prow-robot knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 21, 2020
@nlopezgi
Copy link
Contributor Author

fyi @nachocano

- "watch"
# Our own resources we care about.
- apiGroups:
- "eventing.knative.dev"
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need eventing.knative.dev?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed, thanks for catching, I had just copy-pasted some of these bits from the 0.16.0 version

@nachocano
Copy link
Contributor

Thanks @nlopezgi !!!
Quick question, should we also change their CRDs inside config/ so that they have the v1beta1 storage set to true?

@lionelvillard would you mind taking a quick look as I don't have much experience with migration. Looks good to me but just in case we are missing something...

/approve

@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nachocano, nlopezgi

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 21, 2020
@nlopezgi
Copy link
Contributor Author

Thanks @nlopezgi !!!
Quick question, should we also change their CRDs inside config/ so that they have the v1beta1 storage set to true?

@lionelvillard would you mind taking a quick look as I don't have much experience with migration. Looks good to me but just in case we are missing something...

/approve

If these need to be changed I can also modify them. Most of the changes here were just copying what I see in #3262. Any advice as to what else is needed here is appreciated.

@lionelvillard
Copy link
Member

/hold

We had conversations about not putting the pre and post jobs in subdirectories with the version (in this case v0.18). Instead we should follow what serving is doing.

@vaikas @adamharwayne Do you agree?

@knative-prow-robot knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 21, 2020
@lionelvillard
Copy link
Member

Besides my comment above:
/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 21, 2020
@lionelvillard
Copy link
Member

@Harwayne

@lionelvillard
Copy link
Member

Talked to Ali. Unholding this PR as we can always move this to a different directory in a different PR.

/unhold

@knative-prow-robot knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 25, 2020
@knative-prow-robot knative-prow-robot merged commit 6692b84 into knative:master Aug 25, 2020
@nachocano
Copy link
Contributor

@nlopezgi I think we also need to update the CRDs to mark that we are storing v1beta1 now, so that a fresh install has that, not just the updated clusters.

@nlopezgi
Copy link
Contributor Author

@nlopezgi I think we also need to update the CRDs to mark that we are storing v1beta1 now, so that a fresh install has that, not just the updated clusters.

Is that similar to #3925? If so I'll get started on that

@nachocano
Copy link
Contributor

@nlopezgi I think we also need to update the CRDs to mark that we are storing v1beta1 now, so that a fresh install has that, not just the updated clusters.

Is that similar to #3925? If so I'll get started on that

yes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make ContainerSource v1beta1 the stored version and add a migration tool
9 participants