Skip to content

commands/.../cmdutil: deduplicate updateRoleForResource and support ClusterRole #646

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

Merged
merged 1 commit into from
Oct 22, 2018

Conversation

AlexNPavel
Copy link
Contributor

Description of the change: This moves updateRoleForResource to cmdutil so all of the command libraries can use it and also adds support for ClusterRoles in said function.

Motivation for the change: Deduplication of code and supporting users who want to use ClusterRole

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 19, 2018
@hasbro17
Copy link
Contributor

@shawn-hurley For more context this is to support updating deploy/role.yaml if the type is ClusterRole.
We don't generate that to be a ClusterRole by default but a user may change that.
Some operators using the SDK already do that.
https://github.com/operator-framework/operator-marketplace/blob/master/deploy/rbac.yaml#L1

And we still want the add api command to successfully update the rbac rule for that resource.

@hasbro17
Copy link
Contributor

Seems good to me. Although I'm thinking we should move commands/operator-sdk/cmd/cmdutil/util.go to commands/operator-sdk/cmd/internal/cmdutil/util.go.
Since we don't want to expose these utils outside of the SDK.

@shawn-hurley
Copy link
Member

This is a nit, but I always get worried about util packages.

Avoid meaningless package names. Packages named util, common, or misc provide clients with no sense of what the package contains

https://blog.golang.org/package-names

moving it to internal obviosuly helps but it does appear we have multiple unrelated things happening in this package now. And we should just be careful IMO.

I wonder if this code is better suited in the scaffold, as it is neccessary when you scaffold CR's? Or maybe we make the generation of CRD an Exposed function that the ansible new could call?

@AlexNPavel
Copy link
Contributor Author

@shawn-hurley Do you think this is good for now until we decide how to handle the util packages?

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

@hasbro17
Copy link
Contributor

@shawn-hurley updateRoleForResource() could possibly go in pkg/scaffold/util.go or probably better pkg/scaffold/rbac/rbac.go. Or we can revisit this later if you think it doesn't fit there.

@estroz estroz mentioned this pull request Oct 19, 2018
@AlexNPavel AlexNPavel merged commit f7a8fd3 into operator-framework:master Oct 22, 2018
@AlexNPavel AlexNPavel deleted the multi-role branch October 22, 2018 18:16
@AlexNPavel
Copy link
Contributor Author

We will follow up on moving the cmdutil files in #649

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.

5 participants