add oc create policybinding#8428
Conversation
|
For those not wanting to run with {
"kind":"PolicyBinding",
"apiVersion":"v1",
"metadata":{
"name":"foo:default",
"creationTimestamp":null
},
"lastModified":null,
"policyRef":{
"namespace":"foo",
"name":"default"
},
"roleBindings":[
]
}
|
|
[test] |
89e3c74 to
38f5b39
Compare
|
yes, I think it would help bypass some of the confusion of creating a binding with pure json and trying to figure out the naming quirks. |
|
@stevekuznetsov ptal. |
38f5b39 to
e64adc8
Compare
| return err | ||
| } | ||
|
|
||
| if useShortOutput := o.OutputFormat == "name"; useShortOutput || len(o.OutputFormat) == 0 { |
There was a problem hiding this comment.
I see this is idiomatic to Kubernetes, but man does it take a while to parse.
1b11ad3 to
233f43b
Compare
|
@stevekuznetsov anything else? |
|
@deads2k, no, looks pretty straightforward. Seems like a lot of boilerplate, but I'm not sure we want to invent something to do a generic |
| Example: fmt.Sprintf(policyBindingExample, fullName), | ||
| Run: func(cmd *cobra.Command, args []string) { | ||
| cmdutil.CheckErr(o.Complete(cmd, f, args)) | ||
| cmdutil.CheckErr(o.CreatePolicyBinding()) |
There was a problem hiding this comment.
We should follow k8s guidelines in all oc commands, especially the new ones, so please Complete, Validate and Run :)
There was a problem hiding this comment.
We should follow k8s guidelines in all oc commands, especially the new ones, so please Complete, Validate and Run :)
I should've known that pattern would bite me some day.
There was a problem hiding this comment.
We should follow k8s guidelines in all oc commands, especially the new ones, so please Complete, Validate and Run :)
Done.
233f43b to
aa6a7b4
Compare
|
marking based on @stevekuznetsov's ok. |
| Run: func(cmd *cobra.Command, args []string) { | ||
| cmdutil.CheckErr(o.Complete(cmd, f, args)) | ||
| cmdutil.CheckErr(o.Validate()) | ||
| cmdutil.CheckErr(o.CreatePolicyBinding()) |
There was a problem hiding this comment.
That's still not quite what should be here: Complete, Validate are ok, but the 3rd is Run. 😍
There was a problem hiding this comment.
For what it's worth, I almost like the non-Run function names, at least Run<thing> as when they're all named Run and I want to tell where it's called, I have no way of doing that (go-oracle nonwithstanding). At least with Run<thing> it's grep-able
There was a problem hiding this comment.
@stevekuznetsov Run<thing> is the suggested approach in the guidelines ;)
There was a problem hiding this comment.
@soltysh ah. lol.
I'm amazed they do:
if err := opts.Complete(f, cmd, args, out); err != nil {
cmdutil.CheckErr(err)
}since cmdutil.CheckErr handles the nil case itself.
There was a problem hiding this comment.
@stevekuznetsov yeah, they have still a lot learn from us 😉
aa6a7b4 to
a759435
Compare
|
@deads2k thank you! |
|
@soltysh I like the pattern. First instance of it in the kube or origin repo in a 2014 commit: https://github.com/kubernetes/kubernetes/pull/3275/files#diff-ac0b208737cc45e7d98468252e3097d5R103 . (note the author) :) I have come around to unique names. |
|
@deads2k did we ever come to some consensus on "foo-bar" naming in examples? The rest of the |
this one is special. You aren't allow to choose a name of a policybinding. |
|
@deads2k right, but you can chose the name of a namespace, as in your example: Could be more read-able with something like: |
|
re[test] |
1 similar comment
|
re[test] |
a759435 to
3dd1f51
Compare
3dd1f51 to
222bff8
Compare
|
[merge] |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/3306/) (Image: devenv-rhel7_4047) |
|
Evaluated for origin merge up to 222bff8 |
|
[test] |
|
Evaluated for origin test up to 222bff8 |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/3306/) |
Adds
Usage: oc create policybinding TARGET_POLICY_NAMESPACE [options] Examples: # Create a policy binding in namespace "foo" that references the policy in namespace "bar" $ oc create policybinding bar -n fooThis will allow a cluster-admin to easily create a policybinding object to allow a project admin to bind roles that are not
clusterroles.@pweil- @miheer @nhr Each of you asked about this in the past 24 hours. This makes it easier to follow the directions here https://docs.openshift.com/enterprise/3.1/architecture/additional_concepts/authorization.html#roles that describe the policybinding interplay,
"If you find that these roles do not suit you, a cluster-admin user can create a policyBinding object named <projectname>:default with the CLI using a JSON file. This allows the project admin to bind users to roles that are defined only in the <projectname> local policy."