-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Remove cluster-scoped flag #1434
Remove cluster-scoped flag #1434
Conversation
764d948
to
f7ef2d2
Compare
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.
LGTM after addressing suggestion.
Co-Authored-By: Joe Lanford <[email protected]>
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.
/lgtm
Co-Authored-By: Eric Stroczynski <[email protected]>
New changes are detected. LGTM label has been removed. |
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.
One question otherwise lgtm
log.Info("Generating RBAC rules") | ||
|
||
roleScaffold := &scaffold.Role{ | ||
IsClusterScoped: isClusterScoped, | ||
IsClusterScoped: false, |
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.
Any reason why we couldn't just remove this as well?
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.
@lilic I see what you mean. The default is false but I just wanted to make the default setting explicit for the helm operator since it’s the only one that needs to set this option.
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.
LGTM
Description of the change:
Removal of
--cluster-scoped
flag and consolidation of docs on how to run cluster-scoped operators.Also documents how to enable cluster-scoped CRDs.
The Helm operator will automatically scaffold a ClusterRole/ClusterRoleBinding and inform the user of this if it detects a cluster-scoped resource in the source chart when creating a new project.
Motivation for the change:
There is a general consensus that the
--cluster-scoped
flag does not cover all the use cases of running operators watching different namespaces and expanding it would complicate the CLI and behavior.It's much easier to instruct users to make the small number of changes needed to enable cluster-scoped operators and cluster-scoped CRDs.
With this the default is now namespace-scoped operators.
Fixes: #890