Install from the manifest in a strict(er) order. by Cynocracy · Pull Request #374 · knative/serving-operator · GitHub
Skip to content
This repository was archived by the owner on Jun 24, 2020. It is now read-only.

Install from the manifest in a strict(er) order.#374

Merged
knative-prow-robot merged 3 commits into
knative:masterfrom
Cynocracy:roles-then-bindings
Mar 25, 2020
Merged

Install from the manifest in a strict(er) order.#374
knative-prow-robot merged 3 commits into
knative:masterfrom
Cynocracy:roles-then-bindings

Conversation

@Cynocracy

@Cynocracy Cynocracy commented Mar 25, 2020

Copy link
Copy Markdown
Contributor

This order, Roles -> RoleBindings -> The Rest, prevents the operand from
needing to structure their manifest such that this ordering exists, while
allowing the Operator to use the bootstrapping mechanism described in
#291 to 'escalate' itself
into management of the Knative Serving installation without a *** clusterrole.

Proposed Changes

  • Install roles, then rolebindings, then the remainder of the manifest.

Release Note

NONE

Install from the manifest in a strict order.
8225c47 
This order, Roles -> RoleBindings -> The Rest, prevents the operand from
needing to structure their manifest such that this ordering exists, while
allowing the Operator to use the bootstrapping mechanism described in
knative#291 to 'escalate' itself
into management of the Knative Serving installation without a *** clusterrole.
@Cynocracy Cynocracy changed the title Install from the manifest in a strict order. Install from the manifest in a strict(er) order. Mar 25, 2020
knative-prow-robot
knative-prow-robot reviewed Mar 25, 2020
View reviewed changes

@knative-prow-robot knative-prow-robot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Cynocracy: 0 warnings.

Details

In response to this:

This order, Roles -> RoleBindings -> The Rest, prevents the operand from
needing to structure their manifest such that this ordering exists, while
allowing the Operator to use the bootstrapping mechanism described in
#291 to 'escalate' itself
into management of the Knative Serving installation without a *** clusterrole.

Proposed Changes

  • Install roles, then rolebindings, then the remainder of the manifest.

Release Note

NONE

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@googlebot googlebot added the cla: yes Author(s) signed a CLA. label Mar 25, 2020
mattmoor
mattmoor reviewed Mar 25, 2020
View reviewed changes

@mattmoor mattmoor left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

Comment thread pkg/reconciler/knativeserving/knativeserving_controller.go Outdated
@Cynocracy

Cynocracy commented Mar 25, 2020

Copy link
Copy Markdown
Contributor Author

@Cynocracy Cynocracy mentioned this pull request Mar 25, 2020
Merged
@k4leung4

k4leung4 commented Mar 25, 2020

Copy link
Copy Markdown

/lgtm

jcrossley3
jcrossley3 reviewed Mar 25, 2020
View reviewed changes

@jcrossley3 jcrossley3 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good. Comment on my nit and I'll approve.

Comment thread pkg/reconciler/knativeserving/knativeserving_controller.go Outdated
@Cynocracy

Cynocracy commented Mar 25, 2020

Copy link
Copy Markdown
Contributor Author

Thanks for the review :)

@jcrossley3

jcrossley3 commented Mar 25, 2020

Copy link
Copy Markdown
Contributor

/lgtm
/approve

@knative-prow-robot

knative-prow-robot commented Mar 25, 2020

Copy link
Copy Markdown
Contributor

@knative-prow-robot knative-prow-robot merged commit dfe4a60 into knative:master Mar 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@houshengbo houshengbo Awaiting requested review from houshengbo
3 more reviewers
@jcrossley3 jcrossley3 jcrossley3 left review comments
@mattmoor mattmoor mattmoor left review comments
@knative-prow-robot knative-prow-robot knative-prow-robot left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@jcrossley3 jcrossley3

@k4leung4 k4leung4

Labels

approved cla: yes Author(s) signed a CLA. lgtm size/S

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Cynocracy @k4leung4 @jcrossley3 @knative-prow-robot @mattmoor @googlebot