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

Remove cluster-scoped flag #1434

Merged

Conversation

hasbro17
Copy link
Contributor

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

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 15, 2019
@hasbro17 hasbro17 force-pushed the remove-cluster-scoped-flag branch from 764d948 to f7ef2d2 Compare May 15, 2019 23:45
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 after addressing suggestion.

doc/operator-scope.md Outdated Show resolved Hide resolved
Copy link
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

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

/lgtm

doc/operator-scope.md Outdated Show resolved Hide resolved
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 16, 2019
Co-Authored-By: Eric Stroczynski <[email protected]>
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label May 16, 2019
@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

Copy link
Member

@lilic lilic left a 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,
Copy link
Member

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?

Copy link
Contributor Author

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.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Scaffolding cluster scoped CRDs
6 participants