-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
add oc create clusterquota #9588
Conversation
@enj ptal. |
aa9849e
to
6549bac
Compare
cmdutil.CheckErr(o.Validate()) | ||
cmdutil.CheckErr(o.Run()) | ||
}, | ||
Aliases: []string{"clusterquota"}, |
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 would have this as the name and "clusterresourcequota" as a suggestion but that's just me conforming to kubectl conventions of no aliases. Nevermind that suggestions don't currently work for nested commands.
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 would have this as the name and "clusterresourcequota" as a suggestion but that's just me conforming to kubectl conventions of no aliases. Nevermind that suggestions don't currently work for nested commands.
We've done this for short names before. See oc create deploymentconfig
and oc create dc
. Since the resource shortname is unambiguous I think this is more userfriendly.
@Kargakis anything else on this? Turns out its really handy :) |
No, LGTM |
6549bac
to
ee5e6a8
Compare
[merge] |
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/5543/) (Image: devenv-rhel7_4486) |
Evaluated for origin merge up to ee5e6a8 |
[Test]ing while waiting on the merge queue |
Evaluated for origin test up to ee5e6a8 |
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/5543/) |
return nil | ||
} | ||
|
||
func (o *CreateClusterQuotaOptions) Validate() error { |
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.
Validate
and Run
should be non-pointers method, if following the kubectl conventions.
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.
Validate and Run should be non-pointers method, if following the kubectl conventions.
That's of less utility when your struct contains multiple pointers 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.
The idea is that you should setup whatever needed in Complete
and the others should just use it without ever touching them.
As usual, me too late ;) |
The checks are needed to cover delegation cases. That's why the messages there match the fields. |
Added shortcut for clusterresourcequota called clusterquota to match resourcequota to quota. Added create subcommand.
@derekwaynecarr this incorporates your comments from the previous pull.
--project-selector
--hard
.