fix -version option: print version #367

0xgj · 2018-02-03T08:35:30Z

What this PR does / why we need it:
this is a bug fix. if we using following command

$ ./tf_operator -version

it will not print any version information.

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

Special notes for your reviewer:
NONE @jlewi

Release note:

NONE

This change is

k8s-ci-robot · 2018-02-03T08:35:44Z

Hi @caogj. Thanks for your PR.

I'm waiting for a kubernetes or tensorflow 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.

coveralls · 2018-02-03T08:47:59Z

Coverage increased (+0.1%) to 31.856% when pulling 1d895cc on caogj:fix_versoin_option into e2b13e0 on tensorflow:master.

coveralls · 2018-02-03T08:48:00Z

Coverage remained the same at 31.746% when pulling d9e04e2 on caogj:fix_versoin_option into 0de450f on tensorflow:master.

gaocegege · 2018-02-03T08:53:01Z

cmd/tf_operator/app/server.go

 	glog.Infof("Go Version: %s", runtime.Version())
 	glog.Infof("Go OS/Arch: %s/%s", runtime.GOOS, runtime.GOARCH)
 	if opt.PrintVersion {
+		fmt.Printf("%v\n", version.Version)


Thanks for your PR, but I am not sure if it is a good solution. When you use the command tf-operator -version -logtostderr, It will print

I0203 16:49:35.991400 8213 server.go:73] tf_operator Version: 0.3.0+git I0203 16:49:35.991406 8213 server.go:74] Git SHA: Not provided. I0203 16:49:35.991410 8213 server.go:75] Go Version: go1.8.3 I0203 16:49:35.991412 8213 server.go:76] Go OS/Arch: linux/amd64 0.3.0+git

I prefer to set alsologtostderr to true by default to resue the glog to print version, but if you do not like it, please print all version info by fmt:

fmt.Printf("tf_operator Version: %v", version.Version) fmt.Printf("Git SHA: %s", version.GitSHA) fmt.Printf("Go Version: %s", runtime.Version()) fmt.Printf("Go OS/Arch: %s/%s", runtime.GOOS, runtime.GOARCH)

@gaocegege well, I think glog should be used to print log for debugging; what -version option print is not log, so I think it's fine. :)

fmt.Errorf() formats according to a format specifier and returns the string as a value that satisfies error, and the function will not print any message. And glog.Errorf() logs to the ERROR, WARNING, and INFO logs. Arguments are handled in the manner of fmt.Printf; a newline is appended if missing.

I think they have different behaviour, as for here it is used to return an error so I think it should be fmt 🤔 The error will be printed here: https://github.com/tensorflow/k8s/blob/master/cmd/tf_operator/main.go#L33.

@gaocegege , my bad, you're right. so I think this pr should be fine. 😆

I think version is not enough, maybe

fmt.Printf("tf_operator Version: %v", version.Version) fmt.Printf("Git SHA: %s", version.GitSHA) fmt.Printf("Go Version: %s", runtime.Version()) fmt.Printf("Go OS/Arch: %s/%s", runtime.GOOS, runtime.GOARCH)

is better 🤔 WDYT @caogj

@gaocegege sure. like kubectl version:

$kubectl version Client Version: version.Info{Major:"1", Minor:"8", GitVersion:"v1.8.2", GitCommit:"bdaeafa71f6c7c04636251031f93464384d54963", GitTreeState:"clean", BuildDate:"2017-10-24T19:48:57Z", GoVersion:"go1.8.3", Compiler:"gc", Platform:"darwin/amd64"} Server Version: version.Info{Major:"1", Minor:"8", GitVersion:"v1.8.3+coreos.0", GitCommit:"f96beb2e925b0bb4e6ac0de746c7155dcd8fdc10", GitTreeState:"clean", BuildDate:"2017-11-13T10:18:37Z", GoVersion:"go1.8.3", Compiler:"gc", Platform:"linux/amd64"}

remove unused function PrintVersion

add version.Info()

$./tf_operator -version -logtostderr I0203 18:37:41.892546 16458 server.go:54] EnvKubeflowNamespace not set, use default namespace I0203 18:37:41.892659 16458 server.go:58] Version: 0.3.0+git, Git SHA: Not provided., Go Version: go1.9.2, Go OS/Arch: darwin/amd64 Version: 0.3.0+git, Git SHA: Not provided., Go Version: go1.9.2, Go OS/Arch: darwin/amd64

jlewi · 2018-02-04T18:16:06Z

Thanks for the PR. Can you update the PR description to provide more info about what this PR is doing and why?

jlewi · 2018-02-04T18:14:13Z

cmd/tf_operator/app/server.go

-	glog.Infof("Go OS/Arch: %s/%s", runtime.GOOS, runtime.GOARCH)
+	glog.Info(version.Info())
 	if opt.PrintVersion {
+		fmt.Printf("%s\n", version.Info())


Why are we using fmt here and not glog?
Can you add add a comment explaining why we are using fmt and not glog and also why we are explicitly duplicating the call here even the we call glog.Info(version.info) above.
The code you are adding was removed in an earlier PR so we should add a comment to avoid further thrashing.

+1 for glog.

I think the issue is that glog.info won't print out info level logs by default. So either we need to use fmt.printf or else we would have to modify the log level and turn on --alsologtostderr in the case when we are just printing out the version.

If we use glog.info, then we have the side effect of printing out the file and line number. I'm not sure we really want that in the output of "--version"

@ScorpioCPH @jlewi

I have a discussion with @caogj : #367 (comment)

If we trun on --alsologtostderr as default there will be some additional logs printed when we run tf-operator --version.

Thanks makes sense. Using print seems good to me. But per my other comments can we print the different pieces of information on newlines e.g.
Version: ..
Git Sha: ..

I'd like to but I am not the author of the PR, ping @caogj

jlewi · 2018-02-04T18:15:16Z

version/version.go

-	}
+// get version info
+func Info() string{
+	return fmt.Sprintf("Version: %v, Git SHA: %s, Go Version: %s, Go OS/Arch: %s/%s", Version, GitSHA, runtime.Version(), runtime.GOOS, runtime.GOARCH)


Why not print everything on a new line for easier readability?

jlewi · 2018-02-07T13:31:44Z

/ok-to-test

k8s-ci-robot added the needs-ok-to-test label Feb 3, 2018

gaocegege requested changes Feb 3, 2018

View reviewed changes

gaocegege approved these changes Feb 3, 2018

View reviewed changes

jlewi reviewed Feb 5, 2018

View reviewed changes

k8s-ci-robot removed the needs-ok-to-test label Feb 7, 2018

0xgj force-pushed the fix_versoin_option branch 2 times, most recently from bbf1a38 to df99061 Compare February 7, 2018 15:43

fix -version option: print version

d9e04e2

0xgj force-pushed the fix_versoin_option branch from df99061 to d9e04e2 Compare February 8, 2018 00:55

jlewi approved these changes Feb 8, 2018

View reviewed changes

jlewi merged commit 57567df into kubeflow:master Feb 8, 2018

fix -version option: print version #367

fix -version option: print version #367

Uh oh!

Conversation

0xgj commented Feb 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Feb 3, 2018

Uh oh!

coveralls commented Feb 3, 2018

Uh oh!

coveralls commented Feb 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xgj Feb 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jlewi commented Feb 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jlewi commented Feb 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

0xgj commented Feb 3, 2018 •

edited

Loading

coveralls commented Feb 3, 2018 •

edited

Loading

0xgj Feb 3, 2018 •

edited

Loading