addressing issue #39427 adding a flag --output to 'kubectl version' #39858

alejandroEsc · 2017-01-13T08:03:06Z

What this PR does / why we need it:
Addressing Issue #39427 we all

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #39427

Release note:

kubectl version has new flag --output (=json or yaml) allowing result of the command to be parsed in either json format or yaml.

k8s-reviewable · 2017-01-13T08:03:14Z

This change is

alejandroEsc · 2017-01-13T16:29:19Z

There seems to be some issue upstream, never had these tests fail, local shows things working properly.

alejandroEsc · 2017-01-13T21:22:22Z

@kubernetes/api-reviewers could i get some available eyes here?

liggitt · 2017-01-13T21:32:55Z

pkg/version/version.go

-	GoVersion    string `json:"goVersion"`
-	Compiler     string `json:"compiler"`
-	Platform     string `json:"platform"`
+	Major        string `json:"major,omitempty" yaml:"major"`


don't change this, this is being returned from the /version API endpoint

you mean all these vars? do I make the changes there then?

no, revert this change. it's ok to print empty strings for these fields

liggitt · 2017-01-13T21:33:28Z

pkg/kubectl/cmd/version.go

+}
+
+type ClientAndServerVersionObj struct {
+	ClientVersion version.Info `json:"Client Version" yaml:"Client Version"`


english language keys is odd here... I'd expect something like client and server

I totally agree, was trying to keep it inline with the original keys. I dont want too much variation between what was expected before and what they will get now. What do you think?

for programmatic output, I would go short and lowercase

liggitt · 2017-01-13T21:34:08Z

pkg/kubectl/cmd/version.go

@@ -46,36 +59,92 @@ func NewCmdVersion(f cmdutil.Factory, out io.Writer) *cobra.Command {
 	}
 	cmd.Flags().BoolP("client", "c", false, "Client version only (no server required).")
 	cmd.Flags().BoolP("short", "", false, "Print just the version number.")
+	// default behavior is std.
+	cmd.Flags().String("output", "std", "output format, options available are yaml and json")


leave default empty (not "std") for normal human-readable output?

i was going back and forth on this. I settled on std simply because i thought empty string would be a bit more difficult to understand? Not sure, but i'll change it to empty string if you are ok with that?

liggitt · 2017-01-13T21:35:06Z

pkg/kubectl/cmd/version.go

 	"k8s.io/kubernetes/pkg/kubectl/cmd/templates"
 	cmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util"
 	"k8s.io/kubernetes/pkg/version"
 )

+// We include both struct objects because json marshal does not handle the idea
+// of empty struct objects well and will include and empty server object in results


really? omitempty doesn't work with an empty server struct?

it doesnt. You can try it out, i wasnt happy with that either.

you can make it a pointer to the struct and set it to nil when unused, then

@liggitt ahhh, good point, will try this.

liggitt · 2017-01-13T21:35:54Z

pkg/kubectl/cmd/version.go

-		v = serverVersion.GitVersion
+func printRightFormat(out io.Writer, outputFormat string, vo interface{}) error {
+	if outputFormat == "yaml" {
+		fmt.Println(vo)


this doesn't seem right

:face_palm: arggghhh sorry about this.

alejandroEsc · 2017-01-13T21:44:13Z

@liggitt left some questions there for you just to get some direction, thank you soooo much for taking your time to review, much appreciated!

alejandroEsc · 2017-01-13T21:46:03Z

@liggitt one more question, have you taken a look at the issue ticket, just wanted your opinion on the output now vs older format.

alejandroEsc · 2017-01-13T23:08:45Z

@liggitt made changes requested, please have another look, and thank you again!

liggitt · 2017-01-13T23:20:06Z

pkg/kubectl/cmd/version.go

 	cmd.Flags().MarkShorthandDeprecated("client", "please use --client instead.")
 	return cmd
 }

 func RunVersion(f cmdutil.Factory, out io.Writer, cmd *cobra.Command) error {
-	v := fmt.Sprintf("%#v", version.Get())


I would order it like this:

get the client and (optionally) server versions

build the VersionObj struct with nil set appropriately

switch on cmdutil.GetFlagString(cmd, "output") (don't normalize their arg)

in the "" case, have all the existing output, including short output handling

in the json/yaml cases, marshal and output

in the default case, error

That would be the ideal case. However, the "" case makes it a bit difficult because the output is actually label: version.Info{...} which makes this approach difficult to do. I have to keep the old output because legacy tests etc.. still use this.

Right, in the "" case, include all the old output code

Ok, i think i understand what you are saying... let me try it out. it will most likely be submitted later tonight though, fyi.

k82cn · 2017-01-14T00:42:26Z

pkg/kubectl/cmd/version.go

 		return nil
-	}
+	case of == "yaml" || of == "json":


Please avoid duplicated code of generating version info.

alejandroEsc · 2017-01-14T04:52:37Z

ok @liggitt that was fun, take a look when you are ready and thank you again.

liggitt · 2017-01-14T15:04:22Z

pkg/kubectl/cmd/version.go

-	v := fmt.Sprintf("%#v", version.Get())
+	vo := VersionObj{nil, nil}
+
+	cvg := version.Get()
 	if cmdutil.GetFlagBool(cmd, "short") {


Only handle short output in the human readable case

Can you clarify why this is the case?

The only point of json/yaml output is to consume specific fields programmatically. Truncating to a single field doesn't make that any easier

I disagree, regardless of the consumer we should respect the expectations of the flag.

Then disallow short output combined with json/yaml output

liggitt · 2017-01-14T15:05:20Z

pkg/kubectl/cmd/version.go

-	}
+		serverVersion, err := clientSet.Discovery().ServerVersion()
+		if err != nil {
+			return err


Store this, but don't return it yet. Otherwise a server error prevents us from seeing our client version and is a behavior change

liggitt · 2017-01-14T15:06:02Z

pkg/kubectl/cmd/version.go

-		return err
+		sv := serverVersion
+		if cmdutil.GetFlagBool(cmd, "short") {
+			sv = &version.Info{GitVersion: serverVersion.GitVersion}


Same, don't truncate data here. Truncate in short mode only in the human readable case

liggitt · 2017-01-14T15:07:15Z

pkg/kubectl/cmd/version.go

+	switch of := cmdutil.GetFlagString(cmd, "output"); of {
+	case "":
+		fmt.Fprintf(out, "Client Version: %s\n", fmt.Sprintf("%#v", *vo.ClientVersion))
+		if cmdutil.GetFlagBool(cmd, "client") {


Can just check whether you have a non nil server version

liggitt · 2017-01-14T15:07:28Z

pkg/kubectl/cmd/version.go

+		}
+
+		fmt.Fprintf(out, "Server Version: %s\n", fmt.Sprintf("%#v", *vo.ServerVersion))
+		return nil


Return server error here if you got one

Actually, return server error after the switch statement

alejandroEsc · 2017-01-14T15:22:11Z

Very good points, I'll try to make changes before the end of the day.

alejandroEsc · 2017-03-26T22:42:10Z

@adohe could I get the lgtm applied again? Sorry for the trouble.

alejandroEsc · 2017-03-27T15:01:01Z

@ethernetdan should you remove that donotmerge label and add a 1.7 label instead? also the bot removed the lgtm because of a flake.

eparis · 2017-03-26T23:40:24Z

pkg/kubectl/cmd/version.go

 	"fmt"
-	"io"
-


Why did this get removed. Isn't the import standard

import ( std libs [blank line] other people's packages [blank line] pkg's from kubernetes )

@eparis yeah probably got deleted and re-added after some change, ill go ahead and make the format change.

eparis · 2017-03-28T16:45:02Z

pkg/kubectl/cmd/version.go

 	"fmt"
 	"io"

+	"encoding/json"


encoding/json is part of the standard lib.

ha, i totally missed that during moving things around. thanks for the catch. Could you give this pr and lgtm? i got this dropped after an automatic retest.

… either json or yaml. updating with PR changes requested. latest changes to having short for human readable only, and error cases moved a bit to the end. rebase fixes latest pr. changes. small change moving return nil out of switch. updated the nil check for the error in the humanreadable case. more optimization in humanreadable code. pushed up current test changes, this is purely temporary finished writing tests updated test and function names. changed output extensions from .sh to output. updated version, version struct now just called Version and not VersionObj. made a few changes to testing. fixed testing issues, created better test and cleanup go format change.

0xmichalis · 2017-03-28T17:29:25Z

@alejandroEsc does this also address #43750? Can you post the output of kubectl version with your fix?

0xmichalis · 2017-03-28T17:30:00Z

/lgtm

k8s-github-robot · 2017-03-28T17:30:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adohe, alejandroEsc, deads2k, kargakis

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

~~hack/OWNERS~~ [deads2k]
~~pkg/kubectl/OWNERS~~ [adohe,deads2k]

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

alejandroEsc · 2017-03-28T17:36:55Z

@Kargakis please look at the original ticket: #39427, here i post the output. Thanks again for having a look, hopefully we arent hit with a flake again.

0xmichalis · 2017-03-28T17:42:44Z

Those are looking good but I am asking specifically about the output of the command w/o using -o (the default case)

alejandroEsc · 2017-03-28T17:47:11Z

the default, without any additional output flag, has not changed, the intend here is to keep default behavior and only allow the option to make it human readable if we want it.

0xmichalis · 2017-03-28T17:48:42Z

That's what I thought. Thanks for clarifying. cc: @php-coder

alejandroEsc · 2017-03-28T18:39:28Z

@k8s-bot bazel test this

what is going on with bazel?

ixdy · 2017-03-28T18:41:24Z

Looks like the bazel build is just broken - the upstream busybox dependency no longer exists. cc @mikedanese @spxtr

W0328 18:37:19.565] ERROR: /workspace/k8s.io/kubernetes/build/BUILD:21:1: no such package '@busybox_deb//file': Error downloading [http://ftp.us.debian.org/debian/pool/main/b/busybox/busybox-static_1.22.0-19+b1_amd64.deb] to /root/.cache/bazel/_bazel_root/e9f728bbd90b3fba632eb31b20e1dacd/external/busybox_deb/busybox-static_1.22.0-19+b1_amd64.deb: GET returned 404 Not Found and referenced by '//build:busybox'.
W0328 18:37:19.875] ERROR: Analysis of target '//build/release-tars:release-tars' failed; build aborted.

k8s-github-robot · 2017-03-28T19:12:22Z

Automatic merge from submit-queue

Automatic merge from submit-queue add --output flag to `kubeadm version` ref to kubectl #39858

@php-coder

Automatic merge from submit-queue (batch tested with PRs 46210, 48607, 46874, 46598, 49240) Make "kubectl version" json format output more readable. **What this PR does / why we need it**: ##39858 adds a flag --output to `kubectl version`, but the json format output is displayed in one line. It's not so readable. This PR fixes it. and - adds a shorthand for `output` - ~~refactors that: if `--short` is specified, `--output` will be ignored~~ **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes #43750 **Special notes for your reviewer**: /cc @php-coder @alejandroEsc **Release note**: ```release-note NONE ```

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

k8s-github-robot assigned dchen1107 Jan 13, 2017

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 13, 2017

alejandroEsc force-pushed the ae/issue/39427 branch from 7e5e38a to 5dccc5c Compare January 13, 2017 16:50

liggitt reviewed Jan 13, 2017

View reviewed changes

k82cn reviewed Jan 14, 2017

View reviewed changes

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

liggitt requested changes Jan 14, 2017

View reviewed changes

alejandroEsc force-pushed the ae/issue/39427 branch from facfbe4 to 72742dc Compare January 17, 2017 17:09

liggitt removed their assignment Mar 17, 2017

alejandroEsc force-pushed the ae/issue/39427 branch from f7b3b98 to c01a5a7 Compare March 17, 2017 15:29

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

eparis reviewed Mar 27, 2017

View reviewed changes

alejandroEsc force-pushed the ae/issue/39427 branch from c01a5a7 to a8eb28c Compare March 28, 2017 16:36

eparis reviewed Mar 28, 2017

View reviewed changes

alejandroEsc force-pushed the ae/issue/39427 branch from a8eb28c to a5e6dcb Compare March 28, 2017 17:08

alejandroEsc mentioned this pull request Mar 28, 2017

kubectl version output isn't human readable #43750

Closed

0xmichalis removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Mar 28, 2017

k8s-ci-robot assigned 0xmichalis Mar 28, 2017

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

k8s-github-robot merged commit c01baaf into kubernetes:master Mar 28, 2017

xilabao mentioned this pull request Mar 30, 2017

add --output flag to kubeadm version #43850

Merged

alejandroEsc deleted the ae/issue/39427 branch March 30, 2017 14:06

k8s-github-robot pushed a commit that referenced this pull request Apr 26, 2017

Merge pull request #43850 from xilabao/add-output-to-kubeadm-version

eb0bc85

Automatic merge from submit-queue add --output flag to `kubeadm version` ref to kubectl #39858

xiangpengzhao mentioned this pull request May 29, 2017

Make "kubectl version" json format output more readable. #46598

Merged

addressing issue #39427 adding a flag --output to 'kubectl version' #39858

addressing issue #39427 adding a flag --output to 'kubectl version' #39858

Conversation

alejandroEsc commented Jan 13, 2017 • edited Loading

k8s-reviewable commented Jan 13, 2017

alejandroEsc commented Jan 13, 2017

alejandroEsc commented Jan 13, 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

liggitt Jan 13, 2017 • edited Loading

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

alejandroEsc commented Jan 13, 2017

alejandroEsc commented Jan 13, 2017

alejandroEsc commented Jan 13, 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

alejandroEsc commented Jan 14, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alejandroEsc commented Jan 14, 2017

alejandroEsc commented Mar 26, 2017

alejandroEsc commented Mar 27, 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

0xmichalis commented Mar 28, 2017

0xmichalis commented Mar 28, 2017

k8s-github-robot commented Mar 28, 2017

alejandroEsc commented Mar 28, 2017

0xmichalis commented Mar 28, 2017

alejandroEsc commented Mar 28, 2017 • edited Loading

0xmichalis commented Mar 28, 2017

alejandroEsc commented Mar 28, 2017

ixdy commented Mar 28, 2017

k8s-github-robot commented Mar 28, 2017

alejandroEsc commented Jan 13, 2017 •

edited

Loading

liggitt Jan 13, 2017 •

edited

Loading

alejandroEsc commented Mar 28, 2017 •

edited

Loading