add svcat version #1839
add svcat version #1839
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
Fixes #1752 |
|
Local results: |
cmd/svcat/versions/version_cmd.go
Outdated
| false, | ||
| "Show only the client version", | ||
| ) | ||
| cmd.Flags().BoolVarP( |
There was a problem hiding this comment.
This conflicts with a global kubectl flag, --server, which is why you don't see it output in the plugin manifest yaml file.
I recommend that we follow the kubectl convetion for the version command. Just use a single flag, -c, --client, and always output the client version (since it's always available):
$ kubectl version --help
Print the client and server version information for the current context
Examples:
# Print the client and server versions for the current context
kubectl version
Options:
-c, --client=false: Client version only (no server required).|
|
||
| // ServerVersion asks the Service Catalog API Server for the version.Info object and returns it. | ||
| func (sdk *SDK) ServerVersion() (*version.Info, error) { | ||
| serverVersion, err := sdk.ServiceCatalogClient.Discovery().ServerVersion() |
There was a problem hiding this comment.
Sweet! I didn't know about this! 👍
There was a problem hiding this comment.
Nor did I until I dug in... I was looking at some way to add a new rest endpoint on the generic api server and oh my... that is some crazy code. Then I found the Discovery object and did a dance.
…g and orphan mitigation) (kubernetes-retired#1803)" This reverts commit 4b5d159.
|
PTAL |
| return c.version() | ||
| } | ||
|
|
||
| func (c *versionCmd) version() error { |
There was a problem hiding this comment.
As a follow up we should move this into the pkg/svcat directory. The rule of thumb for cmd/svcat is that it should offload any non-cli logic to that package. But for version it's not a huge deal.
There was a problem hiding this comment.
Left this task as a followup
| @@ -105,6 +106,7 @@ func buildRootCommand() *cobra.Command { | |||
| cmd.AddCommand(newSyncCmd(cxt)) | |||
| cmd.AddCommand(newInstallCmd(cxt)) | |||
| cmd.AddCommand(newTouchCmd(cxt)) | |||
| cmd.AddCommand(versions.NewVersionCmd(cxt)) | |||
There was a problem hiding this comment.
We need to remove the existing handling for the --version flag. So line 114 (printVersion) and lines 81-84. Having it be it's own command is better IMO, so let's just remove the old way.
There was a problem hiding this comment.
Removed --version and updated the install document.
|
|
||
| // ServerVersion asks the Service Catalog API Server for the version.Info object and returns it. | ||
| func (sdk *SDK) ServerVersion() (*version.Info, error) { | ||
| serverVersion, err := sdk.ServiceCatalogClient.Discovery().ServerVersion() |
This added the command
svcat versionwith optional--serveror--clientflags to just return the client (svcat version) or the server (git label via apiserver call).I am not seeing a good test for this, please let me know if you see a testable way to do this.