Service-catalog: Make svcat version command more like kubectl version

Created on 4 Jun 2018  路  6Comments  路  Source: kubernetes-sigs/service-catalog

Make svcat version more in line with kubectl version by always reporting the client version.

Presently svcat reports the following for version info when Kubernetes isn't running:

$ svcat version
Error: unable to get version, Get http://localhost:8080/version: dial tcp [::1]:8080: getsockopt: connection refused
$ svcat version --client
client: v0.1.17-7-gef904a5-dirty

vs kubectl

$ kubectl version
Client Version: version.Info{Major:"1", Minor:"11+", GitVersion:"v1.11.0-alpha.2.8+0d43bdec2b8598-dirty", GitCommit:"0d43bdec2b8598ff542a1afdee876d417b4e7668", GitTreeState:"dirty", BuildDate:"2018-05-16T20:04:10Z", GoVersion:"go1.10.1", Compiler:"gc", Platform:"linux/amd64"}
The connection to the server localhost:8080 was refused - did you specify the right host or port?
help wanted svcat

Most helpful comment

To be clear, svcat version _always_ returns the client version, and the --client flag allows us to query the client version only, in instances where perhaps there isn't a configured cluster yet.

The problem is that when we fail to query the server version, svcat exits immediately before the client version is also printed.

The code that needs to be changed is in the version command

https://github.com/kubernetes-incubator/service-catalog/blob/a514e67a664cff404196158436e6d158a33fc886/cmd/svcat/versions/version_cmd.go#L68-L83

It could be fixed in either of two ways:

  1. Print the error message and do not exit immediately when c.App.ServerVersion() returns an error, so that output.WriteVersion(c.Output, client, server) is still called. The function should still return an error when that call fails however, so that the return code of the svcat version function is still non-zero.
  2. Change the function so that we print out the versions incrementally, first the client version, then the server version.

There aren't existing tests for the version command. As part of this issue, three new tests should be added (that look similar to these new tests that have yet to be merged), that verify the following scenarios using a fake client:

  1. svcat version
  2. svcat version --client
  3. svcat version when the server returns an error

All 6 comments

To be clear, svcat version _always_ returns the client version, and the --client flag allows us to query the client version only, in instances where perhaps there isn't a configured cluster yet.

The problem is that when we fail to query the server version, svcat exits immediately before the client version is also printed.

The code that needs to be changed is in the version command

https://github.com/kubernetes-incubator/service-catalog/blob/a514e67a664cff404196158436e6d158a33fc886/cmd/svcat/versions/version_cmd.go#L68-L83

It could be fixed in either of two ways:

  1. Print the error message and do not exit immediately when c.App.ServerVersion() returns an error, so that output.WriteVersion(c.Output, client, server) is still called. The function should still return an error when that call fails however, so that the return code of the svcat version function is still non-zero.
  2. Change the function so that we print out the versions incrementally, first the client version, then the server version.

There aren't existing tests for the version command. As part of this issue, three new tests should be added (that look similar to these new tests that have yet to be merged), that verify the following scenarios using a fake client:

  1. svcat version
  2. svcat version --client
  3. svcat version when the server returns an error

I thought that this would be a good first PR initially, but since the tests don't exist yet this may be easier for someone already familiar with writing tests in service catalog, or a very enterprising new contributor! 馃榾

Hello :) ! Can i work on this this ?

@A-Hilaly Yup! Go for it 馃檶 When you get to the part about adding tests, feel free to ping back for help. As I noted originally, this part doesn't have existing tests so you will need to add them yourself.

Alright, thank you 馃檹 I'll open a PR soon !

closed by #2109

Was this page helpful?
0 / 5 - 0 ratings

Related issues

carolynvs picture carolynvs  路  8Comments

georgifarashev picture georgifarashev  路  4Comments

carolynvs picture carolynvs  路  8Comments

piotrmiskiewicz picture piotrmiskiewicz  路  3Comments

n3wscott picture n3wscott  路  3Comments