-
Notifications
You must be signed in to change notification settings - Fork 4.7k
add oc create policybinding #8428
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We all ❤️ patterns
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stevekuznetsov Run<thing>
is the suggested approach in the guidelines ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 foo
This 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."