add create subcommand for priorityclass #54858

wackxu · 2017-10-31T11:40:38Z

What this PR does / why we need it:

add create priorityclass sub command

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):Fixes #54857

Special notes for your reviewer:

Release note:

add create priorityclass sub command

k8s-ci-robot · 2017-10-31T11:40:46Z

Hi @wackxu. 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 /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.

I understand the commands that are listed here.

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.

wackxu · 2017-10-31T11:42:14Z

/assign @brendandburns

zjj2wry · 2017-10-31T12:36:45Z

/ok-to-test

wackxu · 2017-11-01T01:54:58Z

/test pull-kubernetes-verify

zjj2wry · 2017-11-01T02:05:24Z

pkg/kubectl/cmd/create_priorityclass.go

+
+	pcExample = templates.Examples(i18n.T(`
+		# Create a priorityclass named high-priority
+		kubectl create priorityclass default-priority --value=1000 --descroption="high priority"


zjj2wry · 2017-11-01T02:07:47Z

pkg/kubectl/cmd/create_priorityclass.go

+		kubectl create priorityclass default-priority --value=1000 --descroption="high priority"
+
+		# Create a priorityclass named default-priority that considered as the global default priority
+		kubectl create priorityclass default-priority --value=1000 --globalDefault=true --descroption="default priority"`))


zjj2wry · 2017-11-01T02:11:10Z

pkg/kubectl/cmd/util/factory_client_access.go

@@ -498,6 +498,7 @@ const (
 	ClusterV1Beta1GeneratorName             = "cluster/v1beta1"
 	PodDisruptionBudgetV1GeneratorName      = "poddisruptionbudget/v1beta1"
 	PodDisruptionBudgetV2GeneratorName      = "poddisruptionbudget/v1beta1/v2"
+	PriorityClassV1GeneratorName            = "priorityclass/v1alpha1"


i think this generator name should be PriorityClassV1Alpha1GeneratorName

zjj2wry · 2017-11-01T02:11:54Z

pkg/kubectl/priorityclass.go

+
+// StructuredGenerate outputs a priorityClass object using the configured fields.
+func (s *PriorityClassV1Generator) StructuredGenerate() (runtime.Object, error) {
+


should be get remove

zjj2wry · 2017-11-01T02:13:28Z

pkg/kubectl/cmd/create_priorityclass_test.go

+	restclient "k8s.io/client-go/rest"
+	"k8s.io/client-go/rest/fake"
+	"k8s.io/kubernetes/pkg/api/legacyscheme"
+	"k8s.io/kubernetes/pkg/apis/scheduling"


use "k8s.io/api/scheduling/......"

zjj2wry · 2017-11-01T02:14:23Z

pkg/kubectl/cmd/create_priorityclass_test.go

+	pcName := "my-pc"
+	f, tf, _, ns := cmdtesting.NewAPIFactory()
+	tf.Client = &fake.RESTClient{
+		GroupVersion:         legacyscheme.Registry.GroupOrDie(scheduling.GroupName).GroupVersion,


legacyscheme have removed from create command

wackxu · 2017-11-01T03:31:59Z

@zjj2wry Thanks for your review, I have addressed all comments, PTAL

brendandburns · 2017-11-01T04:44:23Z

pkg/kubectl/priorityclass.go

@@ -0,0 +1,90 @@
+/*
+Copyright 2016 The Kubernetes Authors.


nit: it's 2017

brendandburns · 2017-11-01T04:45:01Z

pkg/kubectl/priorityclass.go

+
+func (s PriorityClassV1Generator) Generate(params map[string]interface{}) (runtime.Object, error) {
+	err := ValidateParams(s.ParamNames(), params)
+	if err != nil {


nit: inline if check:

if err := ... ; err != nil { ... }

brendandburns · 2017-11-01T04:47:29Z

pkg/kubectl/cmd/create_priorityclass.go

+	cmdutil.AddGeneratorFlags(cmd, cmdutil.PriorityClassV1Alpha1GeneratorName)
+
+	cmd.Flags().Int32("value", 0, i18n.T("the value of this priority class."))
+	cmd.Flags().Bool("globalDefault", false, i18n.T("globalDefault specifies whether this PriorityClass should be considered as the default priority."))


conventions are that flags are dashed not camel case, so this should be

`cmd.Flags().Bool("global-default", ...)

wackxu · 2017-11-01T05:57:49Z

@brendandburns Thanks for your review, I have addressed all comments, PTAL

zjj2wry · 2017-11-01T06:34:02Z

pkg/kubectl/cmd/create_priorityclass.go

+		Example: pcExample,
+		Run: func(cmd *cobra.Command, args []string) {
+			err := CreatePriorityClass(f, cmdOut, cmd, args)
+			cmdutil.CheckErr(err)


cmdutil.CheckErr(CreatePriorityClass(f, cmdOut, cmd, args)) will better

wackxu · 2017-11-01T06:56:19Z

@zjj2wry addressed, PTAL

wackxu · 2017-11-01T07:50:20Z

/test pull-kubernetes-verify

wackxu · 2017-11-01T08:34:34Z

/test pull-kubernetes-verify

zjj2wry · 2017-11-01T09:34:42Z

@wackxu hack/make-rules/../../hack/verify-generated-docs.sh

wackxu · 2017-11-01T11:10:52Z

/test pull-kubernetes-unit

wackxu · 2017-11-01T12:09:25Z

/test pull-kubernetes-unit

wackxu · 2017-11-02T02:34:26Z

@zjj2wry Thanks for you tips and all test has passed, PATL

wackxu · 2017-11-06T08:37:08Z

Kindly ping @zjj2wry @brendandburns @dshulyak for approval

brendandburns · 2017-11-06T23:53:55Z

pkg/kubectl/priorityclass.go

+		return nil, err
+	}
+
+	name, ok := params["name"].(string)


nit: preference for found rather than ok

brendandburns · 2017-11-06T23:54:43Z

pkg/kubectl/priorityclass.go

+
+// StructuredGenerate outputs a priorityClass object using the configured fields.
+func (s *PriorityClassV1Generator) StructuredGenerate() (runtime.Object, error) {
+	if s.Value <= 0 {


Why not move this validation up to the 'Generate' call?

@brendandburns StructuredGenerate and Generate is two different function that can be used to generate object, StructuredGenerate is using pre-configured parameters and Generate is using input parameters. we check if exist in Generate and check if reasonable in StructuredGenerate so that we can reuse the code. I have delete the check because I find that it can be small than zero from the api validation

brendandburns · 2017-11-06T23:55:01Z

Small comments, basically LGTM.

brendandburns · 2017-11-10T19:21:18Z

/lgtm
/approve

k8s-github-robot · 2017-11-10T19:22:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brendandburns, wackxu

Associated issue: 54857

The full list of commands accepted by this bot can be found here.

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

~~docs/OWNERS~~ [brendandburns]
~~pkg/kubectl/OWNERS~~ [brendandburns]

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

fejta-bot · 2017-11-11T11:23:50Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

k8s-github-robot · 2017-11-11T18:45:20Z

Automatic merge from submit-queue (batch tested with PRs 54460, 55258, 54858, 55506, 55510). If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-github-robot assigned dshulyak and eparis Oct 31, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 31, 2017

k8s-ci-robot assigned brendandburns Oct 31, 2017

wackxu force-pushed the createpri branch from c553cf7 to 8c5247c Compare October 31, 2017 12:06

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 31, 2017

wackxu force-pushed the createpri branch from 8c5247c to abec634 Compare October 31, 2017 12:31

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 Oct 31, 2017

zjj2wry reviewed Nov 1, 2017

View reviewed changes

wackxu force-pushed the createpri branch from 5344854 to 55b0f30 Compare November 1, 2017 03:26

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 1, 2017

wackxu force-pushed the createpri branch from 55b0f30 to 91a83de Compare November 1, 2017 03:30

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 1, 2017

brendandburns reviewed Nov 1, 2017

View reviewed changes

pkg/kubectl/priorityclass.go Outdated

@@ -0,0 +1,90 @@

/*

Copyright 2016 The Kubernetes Authors.

Copy link

Contributor

brendandburns Nov 1, 2017

Choose a reason for hiding this comment

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

nit: it's 2017

brendandburns reviewed Nov 1, 2017

View reviewed changes

wackxu force-pushed the createpri branch 2 times, most recently from b3ab4fa to 0ee4803 Compare November 1, 2017 05:56

zjj2wry reviewed Nov 1, 2017

View reviewed changes

wackxu force-pushed the createpri branch from 0ee4803 to 547448d Compare November 1, 2017 06:55

wackxu force-pushed the createpri branch from 547448d to acbe6a4 Compare November 1, 2017 09:53

k8s-github-robot added the kind/old-docs label Nov 1, 2017

brendandburns reviewed Nov 6, 2017

View reviewed changes

add create subcommand for priorityclass

741c58b

wackxu force-pushed the createpri branch from acbe6a4 to 741c58b Compare November 7, 2017 07:03

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 10, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 10, 2017

k8s-github-robot merged commit 0ca74ef into kubernetes:master Nov 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add create subcommand for priorityclass #54858

add create subcommand for priorityclass #54858

wackxu commented Oct 31, 2017

k8s-ci-robot commented Oct 31, 2017

wackxu commented Oct 31, 2017

zjj2wry commented Oct 31, 2017

wackxu commented Nov 1, 2017

zjj2wry Nov 1, 2017

zjj2wry Nov 1, 2017

zjj2wry Nov 1, 2017

zjj2wry Nov 1, 2017

zjj2wry Nov 1, 2017

zjj2wry Nov 1, 2017

wackxu commented Nov 1, 2017

brendandburns Nov 1, 2017

brendandburns Nov 1, 2017

brendandburns Nov 1, 2017

wackxu commented Nov 1, 2017

zjj2wry Nov 1, 2017

wackxu commented Nov 1, 2017

wackxu commented Nov 1, 2017

wackxu commented Nov 1, 2017

zjj2wry commented Nov 1, 2017

wackxu commented Nov 1, 2017

wackxu commented Nov 1, 2017

wackxu commented Nov 2, 2017

wackxu commented Nov 6, 2017

brendandburns Nov 6, 2017

brendandburns Nov 6, 2017

wackxu Nov 7, 2017

brendandburns commented Nov 6, 2017

brendandburns commented Nov 10, 2017

k8s-github-robot commented Nov 10, 2017

fejta-bot commented Nov 11, 2017

k8s-github-robot commented Nov 11, 2017


		// StructuredGenerate outputs a priorityClass object using the configured fields.
		func (s *PriorityClassV1Generator) StructuredGenerate() (runtime.Object, error) {

add create subcommand for priorityclass #54858

add create subcommand for priorityclass #54858

Conversation

wackxu commented Oct 31, 2017

k8s-ci-robot commented Oct 31, 2017

wackxu commented Oct 31, 2017

zjj2wry commented Oct 31, 2017

wackxu commented Nov 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wackxu commented Nov 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wackxu commented Nov 1, 2017

Choose a reason for hiding this comment

wackxu commented Nov 1, 2017

wackxu commented Nov 1, 2017

wackxu commented Nov 1, 2017

zjj2wry commented Nov 1, 2017

wackxu commented Nov 1, 2017

wackxu commented Nov 1, 2017

wackxu commented Nov 2, 2017

wackxu commented Nov 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendandburns commented Nov 6, 2017

brendandburns commented Nov 10, 2017

k8s-github-robot commented Nov 10, 2017

fejta-bot commented Nov 11, 2017

k8s-github-robot commented Nov 11, 2017