Skip to content

updated description when using cluster-scoped flag #1190

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Mar 15, 2019
Merged

updated description when using cluster-scoped flag #1190

merged 2 commits into from
Mar 15, 2019

Conversation

cmendible
Copy link
Contributor

Description of the change:
When using cluster-scoped flag, in the file deploy/role_binding.yaml CluserRole is used instead of Role

Motivation for the change:
Making the docs more accurate

@cmendible
updated description when using cluster-scoped flag
877840d 
When using ` cluster-scoped` flag, in the file `deploy/role_binding.yaml` CluserRole is used instead of Role
@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 9, 2019
@openshift-ci-robot
Copy link

Hi @cmendible. Thanks for your PR.

I'm waiting for a operator-framework or openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@openshift-ci-robot openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 9, 2019
@AlexNPavel
Copy link
Contributor

/test

@AlexNPavel
Copy link
Contributor

/ok-to-test

@openshift-ci-robot openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 11, 2019
AlexNPavel
AlexNPavel previously approved these changes Mar 11, 2019
View reviewed changes
Copy link
Contributor

@AlexNPavel AlexNPavel left a comment

Choose a reason for hiding this comment

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

LGTM

@AlexNPavel
Copy link
Contributor

/hold

I misread part of this PR. The text you are adding (ClusterRole instead of Role) doesn't apply for the deploy/role_binding.yaml manifest but for the deploy/role.yaml manifest, which already has this as a comment.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 11, 2019
@AlexNPavel AlexNPavel self-requested a review March 11, 2019 22:46
@AlexNPavel AlexNPavel dismissed their stale review March 11, 2019 22:46

I misread the PR

@cmendible
Copy link
Contributor Author

cmendible commented Mar 12, 2019 •
edited
Loading

It applies to the roleRef kind in deploy/role_binding.yaml which changes from Role to ClusterRole.

Check https://github.com/operator-framework/operator-sdk/blob/master/pkg/scaffold/rolebinding.go

I can change the text to specify that if needed.

@AlexNPavel
Copy link
Contributor

Yes, I missed that. I think if you specify "for roleRef" at the end of what you currently have now, that should be fine.

/cc @hasbro17 @joelanford

@AlexNPavel
Copy link
Contributor

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 14, 2019
@joelanford
Copy link
Member

I agree with @AlexNPavel re: "for roleRef".

Can you also add the same line in ./doc/ansible/user-guide.md and ./doc/helm/user-guide.md. We have an issue open for consolidating those, but haven't gotten around to it yet.

@cmendible
Improved the documentation
fd1215f 
Changed from: 
Use `ClusterRole` instead of `Role`  
to 
Use `ClusterRole` instead of `Role` for roleRef
@cmendible cmendible mentioned this pull request Mar 14, 2019
Merged
@cmendible
Copy link
Contributor Author

cmendible commented Mar 14, 2019 •
edited
Loading

I think it's ready. Sorry that I created another pull request for the other files... #1209

AlexNPavel
AlexNPavel approved these changes Mar 14, 2019
View reviewed changes
Copy link
Contributor

@AlexNPavel AlexNPavel left a comment

Choose a reason for hiding this comment

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

LGTM

joelanford
joelanford approved these changes Mar 15, 2019
View reviewed changes
Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

LGTM

@joelanford joelanford merged commit afbb6a6 into operator-framework:master Mar 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joelanford joelanford joelanford approved these changes

@estroz estroz Awaiting requested review from estroz

@hasbro17 hasbro17 Awaiting requested review from hasbro17

+1 more reviewer

@AlexNPavel AlexNPavel AlexNPavel approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@cmendible @openshift-ci-robot @AlexNPavel @joelanford