add --output flag to `kubeadm version` #43850

xilabao · 2017-03-30T09:24:28Z

ref to kubectl #39858

k8s-reviewable · 2017-03-30T09:24:34Z

This change is

k8s-ci-robot · 2017-03-30T09:24:35Z

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

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.

luxas

I'd like some changes to this PR thanks

luxas · 2017-03-31T14:10:38Z

cmd/kubeadm/app/cmd/version.go

+		}
+		fmt.Fprintln(out, string(y))
+	default:
+		return errors.New("invalid output format: " + of)


nit, use fmt.Errorf like all other errors

luxas · 2017-03-31T14:11:37Z

cmd/kubeadm/app/cmd/version.go

+		if cmdutil.GetFlagBool(cmd, "short") {
+			fmt.Fprintf(out, "Kubeadm Version: %s\n", clientVersion.GitVersion)
+		} else {
+			fmt.Fprintf(out, "Kubeadm Version: %s\n", fmt.Sprintf("%#v", clientVersion))


nit: I prefer kubeadm version: foo. We don't write Kubeadm anywhere else, I've only seen it lowercased

also, why don't just: fmt.Fprintf(out, "kubeadm version: %#v\n", *v.ClientVersion)?

luxas · 2017-03-31T14:14:31Z

cmd/kubeadm/app/cmd/version.go

 	"k8s.io/kubernetes/pkg/version"
 )

+type Version struct {
+	ClientVersion *apimachineryversion.Info `json:"kubeadmVersion,omitempty" yaml:"kubeadmVersion,omitempty"`


Why not clientVersion in json? I don't think the extra yaml field is required.

luxas · 2017-03-31T14:14:55Z

cmd/kubeadm/app/cmd/version.go

@@ -35,10 +44,39 @@ func NewCmdVersion(out io.Writer) *cobra.Command {
 			kubeadmutil.CheckErr(err)
 		},
 	}
+	cmd.Flags().BoolP("short", "", false, "Print just the version number.")


only print the version (tag|number)

luxas · 2017-03-31T14:15:10Z

cmd/kubeadm/app/cmd/version.go

 	"k8s.io/kubernetes/pkg/version"
 )

+type Version struct {


godoc comment

ping, please add a godoc comment about this struct

luxas · 2017-03-31T14:16:04Z

cmd/kubeadm/app/cmd/version.go

 	return cmd
 }

 func RunVersion(out io.Writer, cmd *cobra.Command) error {
-	fmt.Fprintf(out, "kubeadm version: %#v\n", version.Get())
+	v := Version{}


v := Version{ ClientVersion: &version.Get(), }

and then use v.ClientVersion

xilabao · 2017-04-03T03:15:14Z

Done. @luxas

luxas · 2017-04-03T15:36:46Z

cmd/kubeadm/app/cmd/version.go

 	"k8s.io/kubernetes/pkg/version"
 )

+type Version struct {


ping, please add a godoc comment about this struct

luxas · 2017-04-03T15:44:43Z

cmd/kubeadm/app/cmd/version.go

+	switch of := cmdutil.GetFlagString(cmd, "output"); of {
+	case "":
+		if cmdutil.GetFlagBool(cmd, "short") {
+			fmt.Fprintf(out, "kubeadm version: %s\n", v.clientVersion.GitVersion)


if the user really wants the short version, we should probably drop kubeadm version here

luxas · 2017-04-03T15:45:18Z

cmd/kubeadm/app/cmd/version.go

@@ -35,10 +44,39 @@ func NewCmdVersion(out io.Writer) *cobra.Command {
 			kubeadmutil.CheckErr(err)
 		},
 	}
+	cmd.Flags().BoolP("short", "", false, "only print the version (tag|number)")
+	cmd.Flags().StringP("output", "o", "", "output format, options available are yaml and json")


didn't check how kubectl does it, but to be -o short would be better than two flags

Should we keep the same format with kubectl? I think your solution also make sense.

luxas · 2017-04-03T15:46:10Z

cmd/kubeadm/app/cmd/version.go

 	"k8s.io/kubernetes/pkg/version"
 )

+type Version struct {
+	// clientVersion contains the kubeadm version.
+	clientVersion *apimachineryversion.Info `json:"kubeadmVersion,omitempty" yaml:"kubeadmVersion,omitempty"`


ping: why not clientVersion here? Also the yaml statement is unneeded, we only do json everywhere else.
(the yaml parser will work anyway)

someone else may want the yaml statement #43750

But the yaml statement here defaults to the json value; the duplication here is unneded.
I think this should just be: clientVersion *apimachineryversion.Info json:"clientVersion"`

luxas

please address the three comments that are left

luxas · 2017-04-05T11:03:57Z

cmd/kubeadm/app/cmd/version.go

 	"k8s.io/kubernetes/pkg/version"
 )

+type Version struct {
+	// clientVersion contains the kubeadm version.
+	clientVersion *apimachineryversion.Info `json:"kubeadmVersion,omitempty" yaml:"kubeadmVersion,omitempty"`


But the yaml statement here defaults to the json value; the duplication here is unneded.
I think this should just be: clientVersion *apimachineryversion.Info json:"clientVersion"`

luxas

Ok this LGTM now.

But I think we should discuss -o short vs --short. I'm in favor of -o short

cc @jbeda @philips @kubernetes/sig-cli-pr-reviews

luxas · 2017-04-05T13:58:20Z

(but hmm, I'd love some tests for this, please CLI tests for this in cmd/kubeadm/app/test)
After that, real LGTM.

/approve

dmmcquay · 2017-04-13T17:24:31Z

@xilabao Can you add tests and I'll apply my lgtm.

grodrigues3 · 2017-04-13T18:21:37Z

@k8s-bot ok to test

xilabao · 2017-04-17T09:36:16Z

@k8s-bot verify test this

dmmcquay · 2017-04-17T21:01:25Z

@xilabao Please fix errors with verification.

@luxas do we still need to discuss the -o short vs --short?

luxas · 2017-04-17T21:05:59Z

Derek: Preferably, yes. What's your take?

…

On 18 Apr 2017, at 00:01, Derek McQuay ***@***.***> wrote: @xilabao Please fix errors with verification. @luxas do we still need to discuss the -o short vs --short? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

dmmcquay · 2017-04-18T05:12:40Z

I prefer -o short as well.

dmmcquay · 2017-04-18T18:45:27Z

@xilabao I think this is shaping up nicely.

Do you mind writing tests that capture the output of running the test cmd and running a regex to make sure the output is sane? Something like this: https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/test/cmd/token_test.go

xilabao · 2017-04-19T04:59:57Z

@dmmcquay PTAL

dmmcquay

This looks much better. Just small comment about the tests.

dmmcquay · 2017-04-19T16:27:24Z

cmd/kubeadm/test/cmd/version_test.go

+const (
+	ShortExpectedRegex  = "^v.+\n$"
+	JsonExpectedRegex   = "^{\"kubeadmVersion\":{\"major\":\".+\",\"minor\":\".+\",\"gitVersion\":\".+\",\"gitCommit\":\".+\",\"gitTreeState\":\".+\",\"buildDate\":\".+\",\"goVersion\":\".+\",\"compiler\":\".+\",\"platform\":\".+\"}}\n$"
+	YamlExpectedRegex   = "^kubeadmVersion:\n\\s+buildDate:.+\n\\s+compiler:.+\n\\s+gitCommit:.+\n\\s+gitTreeState:.+\n\\s+gitVersion:.+\n\\s+goVersion:.+\n\\s+major:.+\n\\s+minor:.+\n\\s+platform:.+\n\n$"


I think instead of having a regex for json and yaml, you could just marshal it and check it that way? The json and yaml regex seems a bit intense.

xilabao · 2017-04-20T08:20:24Z

@dmmcquay PTAL

dmmcquay · 2017-04-20T16:16:09Z

/lgtm

thanks for going back and forth on this one.

luxas

LGTM other than kubeadmVersion, I still think that should be clientVersion

cc @kubernetes/sig-cli-pr-reviews

luxas · 2017-04-20T16:30:02Z

cmd/kubeadm/app/cmd/version.go

 	"k8s.io/kubernetes/pkg/version"
 )

+// Version provides the version information of kubeadm.
+type Version struct {
+	ClientVersion *apimachineryversion.Info `json:"kubeadmVersion"`


still I don't understand why this is kubeadmVersion instead of clientVersion?
kubeadm is a client and I think it's easiest if it has the same schema as kubectl

kubectl version -o json use clientVersion to keep consistent with kubectl version . I saw the output of kubeadm version is kubeadm version... Should we change all of them?

I mean that I think this line should be

ClientVersion *apimachineryversion.Info `json:"clientVersion"`

luxas

LGTM, thanks!

luxas · 2017-04-26T09:16:26Z

/lgtm

k8s-github-robot · 2017-04-26T09:16:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dmmcquay, luxas, xilabao

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

~~cmd/kubeadm/OWNERS~~ [luxas]

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 · 2017-04-26T10:16:51Z

Automatic merge from submit-queue

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 30, 2017

k8s-github-robot assigned dmmcquay Mar 30, 2017

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Mar 30, 2017

luxas suggested changes Mar 31, 2017

View reviewed changes

luxas changed the title ~~add -- output to kubeadm version~~ add --output flag to kubeadm version Mar 31, 2017

luxas self-assigned this Mar 31, 2017

luxas added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Mar 31, 2017

xilabao force-pushed the add-output-to-kubeadm-version branch from e0aa692 to 78b017b Compare April 3, 2017 03:12

luxas reviewed Apr 3, 2017

View reviewed changes

luxas reviewed Apr 5, 2017

View reviewed changes

xilabao force-pushed the add-output-to-kubeadm-version branch from 78b017b to cc27c66 Compare April 5, 2017 11:44

luxas approved these changes Apr 5, 2017

View reviewed changes

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

grodrigues3 added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 5, 2017

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 Apr 13, 2017

xilabao force-pushed the add-output-to-kubeadm-version branch 2 times, most recently from 434dec8 to 38e331e Compare April 17, 2017 05:08

xilabao force-pushed the add-output-to-kubeadm-version branch from 38e331e to 93ab6fd Compare April 18, 2017 03:07

xilabao force-pushed the add-output-to-kubeadm-version branch 3 times, most recently from 6101c6c to 6039f4c Compare April 18, 2017 09:47

xilabao force-pushed the add-output-to-kubeadm-version branch from 6039f4c to d26182f Compare April 19, 2017 03:24

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 19, 2017

dmmcquay suggested changes Apr 19, 2017

View reviewed changes

xilabao force-pushed the add-output-to-kubeadm-version branch from d26182f to bb15fec Compare April 20, 2017 03:14

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

dmmcquay approved these changes Apr 20, 2017

View reviewed changes

luxas reviewed Apr 20, 2017

View reviewed changes

luxas added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Apr 20, 2017

add output flag to kubeadm version

3719840

xilabao force-pushed the add-output-to-kubeadm-version branch from bb15fec to 3719840 Compare April 26, 2017 08:51

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 26, 2017

luxas approved these changes Apr 26, 2017

View reviewed changes

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

luxas removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Apr 26, 2017

k8s-github-robot merged commit eb0bc85 into kubernetes:master Apr 26, 2017

add --output flag to kubeadm version #43850

add --output flag to kubeadm version #43850

Conversation

xilabao commented Mar 30, 2017

k8s-reviewable commented Mar 30, 2017

k8s-ci-robot commented Mar 30, 2017

luxas left a 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

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

xilabao commented Apr 3, 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

Choose a reason for hiding this comment

luxas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luxas left a comment

Choose a reason for hiding this comment

luxas commented Apr 5, 2017

dmmcquay commented Apr 13, 2017

grodrigues3 commented Apr 13, 2017

xilabao commented Apr 17, 2017

dmmcquay commented Apr 17, 2017

luxas commented Apr 17, 2017 via email

dmmcquay commented Apr 18, 2017

dmmcquay commented Apr 18, 2017

xilabao commented Apr 19, 2017

dmmcquay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xilabao commented Apr 20, 2017

dmmcquay commented Apr 20, 2017

luxas left a 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

luxas left a comment

Choose a reason for hiding this comment

luxas commented Apr 26, 2017

k8s-github-robot commented Apr 26, 2017

k8s-github-robot commented Apr 26, 2017

add --output flag to `kubeadm version` #43850

add --output flag to `kubeadm version` #43850