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

Upgrade script for patching broker annotation #2910

Merged
merged 8 commits into from
Apr 6, 2020

Conversation

vaikas
Copy link
Contributor

@vaikas vaikas commented Apr 3, 2020

Fixes #2840

Proposed Changes

  • Introduce a script that runs as a job that will patch all Brokers without BrokerClass annotation to ChannelBasedBroker

Release Note

- 🐛 Fix bug
* action required * You have to run the provided upgrade script, or something like this to update all the Brokers with the proper BrokerClass or they will not continue to be reconciled after 0.14 cuts.
Details for the script are in:
config/upgrade/v0.14.0/README.md
Release artifact is:
https://github.com/knative/eventing/releases

Docs

@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 Apr 3, 2020
@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Apr 3, 2020
@knative-prow-robot knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 3, 2020
@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 3, 2020
Copy link
Member

@mattmoor mattmoor left a comment

Choose a reason for hiding this comment

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

Produced via:
prettier --write --prose-wrap=always $(find -name '*.md' | grep -v vendor | grep -v .github | grep -v docs/cmd/)

@knative-prow-robot knative-prow-robot added the area/test-and-release Test infrastructure, tests or release label Apr 3, 2020
Copy link
Member

@mattmoor mattmoor left a comment

Choose a reason for hiding this comment

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

Produced via:
prettier --write --prose-wrap=always $(find -name '*.md' | grep -v vendor | grep -v .github | grep -v docs/cmd/)

@grantr
Copy link
Contributor

grantr commented Apr 3, 2020

/cc @liu-cong

Copy link
Member

@mattmoor mattmoor left a comment

Choose a reason for hiding this comment

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

Produced via:
prettier --write --prose-wrap=always $(find -name '*.md' | grep -v vendor | grep -v .github | grep -v docs/cmd/)

@vaikas vaikas marked this pull request as ready for review April 3, 2020 22:56
@vaikas vaikas changed the title [WIP] upgrade script for patching broker annotation Upgrade script for patching broker annotation Apr 3, 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 Apr 3, 2020
Copy link
Member

@mattmoor mattmoor left a comment

Choose a reason for hiding this comment

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

Produced via:
prettier --write --prose-wrap=always $(find -name '*.md' | grep -v vendor | grep -v .github | grep -v docs/cmd/)

If you installed to different namespace, you need to modify the upgrade.yaml
appropriately. Also the job by default runs as `eventing-controller` service
account, you can also modify that but the service account will need to have
permissions to list `Namespace`s, list and patch `Broker`s.
Copy link
Member

Choose a reason for hiding this comment

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

Format markdown:

Suggested change
permissions to list `Namespace`s, list and patch `Broker`s.
permissions to list `Namespace`s, list and patch `Broker`s.

annotation like so.

```
annotations:
Copy link
Member

Choose a reason for hiding this comment

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

@k4leung4 I think this is someting that the operator should do
/cc @aliok

@matzew
Copy link
Member

matzew commented Apr 6, 2020

will this be removed after the release ?

Copy link
Contributor

@liu-cong liu-cong left a comment

Choose a reason for hiding this comment

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

a few nits plus a couple of places where errors need to be handled.

pkg/upgrader/broker/v0.14.0/upgrader.go Outdated Show resolved Hide resolved
pkg/upgrader/broker/v0.14.0/upgrader.go Outdated Show resolved Hide resolved
cmd/upgrade/v0.14.0/main.go Outdated Show resolved Hide resolved
pkg/upgrader/broker/v0.14.0/upgrader.go Show resolved Hide resolved
pkg/upgrader/broker/v0.14.0/upgrader.go Outdated Show resolved Hide resolved
pkg/upgrader/broker/v0.14.0/upgrader.go Outdated Show resolved Hide resolved
pkg/upgrader/broker/v0.14.0/upgrader_test.go Outdated Show resolved Hide resolved
pkg/upgrader/broker/v0.14.0/upgrader_test.go Show resolved Hide resolved
pkg/upgrader/broker/v0.14.0/upgrader_test.go Outdated Show resolved Hide resolved
pkg/upgrader/broker/v0.14.0/upgrader.go Outdated Show resolved Hide resolved
Copy link
Contributor

@grantr grantr left a comment

Choose a reason for hiding this comment

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

I'm not sure when this should be removed, but since it's versioned I don't think it's required to remove it.

Thank you for writing such comprehensive unit tests! 😍

/lgtm
/hold
Holding for other comments, but I'd like to get this in by pre-release cutoff (tomorrow) so we can all start testing it.

@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 Apr 6, 2020
@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 6, 2020
@grantr
Copy link
Contributor

grantr commented Apr 6, 2020

@vaikas do we need a docs PR too? Since the install docs link directly to release artifacts, anyone following the install instructions to upgrade won't see that this is required. To be fair that seems like a general docs problem and not specific to this issue.

@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 6, 2020
@vaikas
Copy link
Contributor Author

vaikas commented Apr 6, 2020

I don't see a need to remove it after the release, that's why I versioned it. But thoughts?

@vaikas
Copy link
Contributor Author

vaikas commented Apr 6, 2020

@liu-cong thanks, I believe I addressed all comments, PTAL.

@grantr Seems like we should have a separate "upgrade" section and I don't see a place for it, seems like in the past the ReleaseNotes has been a place where we've tacked things that require attention and from what I understand that's been working?

@knative-metrics-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-knative-eventing-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/upgrader/broker/v0.14.0/upgrader.go Do not exist 77.4%

@grantr
Copy link
Contributor

grantr commented Apr 6, 2020

/hold cancel

Seems like we should have a separate "upgrade" section

Agreed. I'll open an issue in docs.

from what I understand that's been working?

I don't know if it's working or not, but I guess we haven't had many complaints. 😃

@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 Apr 6, 2020
Copy link
Contributor

@grantr grantr left a comment

Choose a reason for hiding this comment

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

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 6, 2020
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: grantr, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@liu-cong
Copy link
Contributor

liu-cong commented Apr 6, 2020

/lgtm

@knative-prow-robot knative-prow-robot merged commit 9bf9bb5 into knative:master Apr 6, 2020
@vaikas vaikas deleted the upgrade-script branch April 6, 2020 21:41
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. area/test-and-release Test infrastructure, tests or release 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.

Upgrade to class based broker controller
8 participants