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

Added kubectl create clusterrole command. #41538

Merged
merged 1 commit into from
Feb 23, 2017

Conversation

xingzhou
Copy link
Contributor

Added kubectl create clusterrole command.

Fixed part of #39596

Special notes for your reviewer:
@deads2k, please help to review this patch, thanks

Release note:

   Added one new command `kubectl create clusterrole` to help user create a single ClusterRole from command line.

@k8s-ci-robot
Copy link
Contributor

Hi @xingzhou. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot 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.

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. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 16, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-github-robot k8s-github-robot added kind/old-docs size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Feb 16, 2017

// Remove duplicate verbs.
verbs := []string{}
for _, v := range c.Verbs {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you factor out some of this duplicate logic from the two commands?


clusterRole := &rbac.ClusterRole{}

// Create separate rule for each of the api group.
Copy link
Contributor

Choose a reason for hiding this comment

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

Creating the resource policy rules should be common code between create role and create cluster role.

}

// Split verbs to resource verbs and non-resource verbs
resourceVerbs, nonResourceVerbs := []string{}, []string{}
Copy link
Contributor

Choose a reason for hiding this comment

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

you aren't going to be able to distinguish

cmd.Flags().StringSliceVar(&c.Verbs, "verb", []string{}, "verb that applies to the resources contained in the rule")
cmd.Flags().StringSlice("resource", []string{}, "resource that the rule applies to")
cmd.Flags().StringSliceVar(&c.ResourceNames, "resource-name", []string{}, "resource in the white list that the rule applies to")
cmd.Flags().StringSliceVar(&c.NonResourceURLs, "non-resource-url", []string{}, "non-resource URL is a partial URL that a user should have access to")
Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking about this, I suspect we actually want to defer this option until later. We may even say its only on the set command. The vast majority of usage is interested in resource access.

@deads2k
Copy link
Contributor

deads2k commented Feb 16, 2017

I think narrowing the scope and DRYing it out a little bit will make this work out nicely.

@bgrant0607 bgrant0607 assigned deads2k and unassigned smarterclayton Feb 16, 2017
@bgrant0607
Copy link
Member

cc @kubernetes/sig-cli-pr-reviews

@xingzhou
Copy link
Contributor Author

David, I understand that we want to keep kubectl create series command simple, but seems nonresourceurl is the only different options to role options that I can see. Do we plan to add it some day in the future or we just rely on set command to modify the existing cluster roles? Since if we remove nonresourceurl, in terms of options, there won't be much difference between cluster role and role, and most of the complete/validate/run code can be extract to a common place.

@deads2k
Copy link
Contributor

deads2k commented Feb 17, 2017

Do we plan to add it some day in the future or we just rely on set command to modify the existing cluster roles?

Yes, I think we will want to add it, but we need to consider how it interacts with things like verbs. I think from a structure perspective, you could build the resource rules in a common fashion and then later add the non-resource rules, so we aren't boxed into a corner either way.

}

func (c *CreateClusterRoleOptions) RunCreateClusterRole(f cmdutil.Factory, cmdOut io.Writer, cmd *cobra.Command, args []string) error {
mapper, _ := f.Object()

Choose a reason for hiding this comment

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

actually I would not expect to depends on cobra on the run logical.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adohe, any code patterns need to follow here? extract the arguments from cmd object and pass them in as parameters? wonder any examples in current commands

Copy link
Contributor

Choose a reason for hiding this comment

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

@adohe, any code patterns need to follow here? extract the arguments from cmd object and pass them in as parameters? wonder any examples in current commands

I missed this. Yeah, Complete should gather this and set it in the options.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 20, 2017
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 20, 2017
@xingzhou
Copy link
Contributor Author

Have removed non-resource-url option now. Keep CreateClusterRole struct in case we insert non-resource-url in the future, PTAL. thx

@deads2k
Copy link
Contributor

deads2k commented Feb 20, 2017

/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 20, 2017
@deads2k deads2k removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 20, 2017
return cmd
}

func (c *CreateClusterRoleOptions) RunCreateRole(f cmdutil.Factory, cmdOut io.Writer, cmd *cobra.Command, args []string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

As @adohe pointed out. Don't pass arguments here. Instead, use Complete to gather the bits you need and store the on your options.

Added `kubectl create clusterrole` command.
@xingzhou
Copy link
Contributor Author

Have tried to move all the parameters from Run method to CreateRoleOptions, please take a look ,thanks.

@@ -50,20 +53,30 @@ type CreateRoleOptions struct {
Verbs []string
Resources []schema.GroupVersionResource
ResourceNames []string

DryRun bool
OutputFormat string
Copy link
Contributor

Choose a reason for hiding this comment

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

At some point, I think we'll need to start wrapping this logic inside of the PrintObject function. You don't need to mess with it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, those are just common command options, not specific to this one. Do we have plan now, please let me know if I can help, thanks!

@deads2k
Copy link
Contributor

deads2k commented Feb 22, 2017

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 22, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

The following people have approved this PR: deads2k, xingzhou

Needs approval from an approver in each of these OWNERS Files:

We suggest the following people:
cc @pwittrock @eparis
You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@deads2k deads2k added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 22, 2017
@deads2k
Copy link
Contributor

deads2k commented Feb 22, 2017

@k8s-bot unit test this: issue #41885

@deads2k
Copy link
Contributor

deads2k commented Feb 22, 2017

@k8s-bot gci gce e2e test this

@deads2k
Copy link
Contributor

deads2k commented Feb 22, 2017

@k8s-bot non-cri e2e test this: issue #41889

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 41146, 41486, 41482, 41538, 41784)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. 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.

8 participants