Skip to content

support NonResourceURL for kubectl create clusterrole #45809

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
k8s-github-robot merged 1 commit into from
May 26, 2017
Merged

support NonResourceURL for kubectl create clusterrole #45809

k8s-github-robot merged 1 commit into from
May 26, 2017
+80 −5

Conversation

@CaoShuFeng
Copy link
Contributor

@CaoShuFeng CaoShuFeng commented May 15, 2017 •
edited
Loading

Release note:

add --non-resource-url to kubectl create clusterrole

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 15, 2017
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels May 15, 2017
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented May 15, 2017

Hi @CaoShuFeng. 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.

Details

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 needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 15, 2017
@eparis
Copy link
Contributor

eparis commented May 16, 2017

@k8s-bot ok to test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 16, 2017
@eparis eparis added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels May 16, 2017
liggitt
liggitt reviewed May 16, 2017
View reviewed changes
pkg/kubectl/cmd/create_clusterrole.go Outdated
Copy link
Member

@liggitt liggitt May 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

--prefix is misleading... /foo does not give access to /foo/bar. I would call this --non-resource-url

Copy link
Contributor Author

@CaoShuFeng CaoShuFeng May 17, 2017

Choose a reason for hiding this comment

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

Done.

@CaoShuFeng CaoShuFeng force-pushed the non-resource-url-create-rolebinding branch from 6a094ac to ffc31a8 Compare May 17, 2017 08:13
@CaoShuFeng
Copy link
Contributor Author

CaoShuFeng commented May 17, 2017

@k8s-bot pull-kubernetes-federation-e2e-gce test this

2 similar comments
@CaoShuFeng
Copy link
Contributor Author

CaoShuFeng commented May 17, 2017

@k8s-bot pull-kubernetes-federation-e2e-gce test this

@CaoShuFeng
Copy link
Contributor Author

CaoShuFeng commented May 22, 2017

@k8s-bot pull-kubernetes-federation-e2e-gce test this

liggitt
liggitt reviewed May 23, 2017
View reviewed changes
pkg/kubectl/cmd/create_role.go Outdated
Copy link
Member

@liggitt liggitt May 23, 2017

Choose a reason for hiding this comment

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

verbs are going to be tricky if you are trying to create a role containing both resources and non-resource urls (the verbs that apply to those are different... see https://kubernetes.io/docs/admin/authorization/#review-your-request-attributes)

Copy link
Contributor Author

@CaoShuFeng CaoShuFeng May 25, 2017

Choose a reason for hiding this comment

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

done.

liggitt
liggitt reviewed May 23, 2017
View reviewed changes
hack/make-rules/test-cmd-util.sh Outdated
Copy link
Member

@liggitt liggitt May 23, 2017

Choose a reason for hiding this comment

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

surprised the * didn't need quoting

@liggitt
Copy link
Member

liggitt commented May 23, 2017

@kubernetes/sig-cli-pr-reviews

@k8s-ci-robot k8s-ci-robot added the sig/cli Categorizes an issue or PR as relevant to SIG CLI. label May 23, 2017
@CaoShuFeng CaoShuFeng force-pushed the non-resource-url-create-rolebinding branch from ffc31a8 to 301cec6 Compare May 23, 2017 12:44
liggitt
liggitt reviewed May 25, 2017
View reviewed changes
pkg/kubectl/cmd/create_clusterrole.go Outdated
Copy link
Member

@liggitt liggitt May 25, 2017

Choose a reason for hiding this comment

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

include patch, head, and options

Copy link
Contributor Author

@CaoShuFeng CaoShuFeng May 25, 2017

Choose a reason for hiding this comment

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

Got it.

Copy link
Contributor Author

@CaoShuFeng CaoShuFeng May 25, 2017

Choose a reason for hiding this comment

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

Done.

liggitt
liggitt reviewed May 25, 2017
View reviewed changes
pkg/kubectl/cmd/create_role.go Outdated
Copy link
Member

@liggitt liggitt May 25, 2017

Choose a reason for hiding this comment

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

all the nonResourceURLs can go in a single rule

Copy link
Contributor Author

@CaoShuFeng CaoShuFeng May 25, 2017

Choose a reason for hiding this comment

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

Good idea.

Copy link
Contributor Author

@CaoShuFeng CaoShuFeng May 25, 2017

Choose a reason for hiding this comment

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

Done.

@CaoShuFeng CaoShuFeng force-pushed the non-resource-url-create-rolebinding branch from 301cec6 to a65052d Compare May 25, 2017 09:27
liggitt
liggitt reviewed May 25, 2017
View reviewed changes
pkg/kubectl/cmd/create_clusterrole.go Outdated
Copy link
Member

@liggitt liggitt May 25, 2017

Choose a reason for hiding this comment

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

depending on an error condition this way is a little odd... I'd factor out a validateResources() helper and use it from both Validate() methods, rather than delegate to CreateRoleOptions.Validate() and ignore specific returned errors. You'll end up duplicating the empty name and verbs check but that's not a big deal.

Copy link
Contributor Author

@CaoShuFeng CaoShuFeng May 26, 2017

Choose a reason for hiding this comment

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

Done.

@CaoShuFeng CaoShuFeng force-pushed the non-resource-url-create-rolebinding branch from a65052d to 93e50b1 Compare May 26, 2017 04:22
@liggitt
Copy link
Member

liggitt commented May 26, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 26, 2017
@deads2k
Copy link
Contributor

deads2k commented May 26, 2017

/approve

@k8s-github-robot
Copy link

k8s-github-robot commented May 26, 2017

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CaoShuFeng, deads2k, liggitt

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

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 26, 2017
@k8s-github-robot
Copy link

k8s-github-robot commented May 26, 2017

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 899b6c0 into kubernetes:master May 26, 2017
@CaoShuFeng CaoShuFeng mentioned this pull request May 31, 2017
Merged
dims pushed a commit to dims/kubernetes that referenced this pull request Jun 2, 2017
Merge pull request kubernetes#46701 from CaoShuFeng/unit_clusterrole_…
b7a424f 
…binding

Automatic merge from submit-queue (batch tested with PRs 46432, 46701, 46326, 40848, 46396)

add some unit tests for "kubectl create clusterrole"

kubernetes#45809 adds support for non-resource-url to "kubectl create clusterrole"
This pr add some unit test for kubernetes#45809 

**Release note**:

```
NONE
```
@CaoShuFeng CaoShuFeng deleted the non-resource-url-create-rolebinding branch November 6, 2017 05:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@liggitt liggitt liggitt left review comments

Assignees

@liggitt liggitt

@smarterclayton smarterclayton

@eparis eparis

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. sig/cli Categorizes an issue or PR as relevant to SIG CLI. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@CaoShuFeng @k8s-ci-robot @eparis @liggitt @deads2k @k8s-github-robot @smarterclayton