Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

don't use base cmd name when printing client version #12781

Merged
merged 1 commit into from
Mar 13, 2017
Merged

don't use base cmd name when printing client version #12781

merged 1 commit into from
Mar 13, 2017

Conversation

juanvallejo
Copy link
Contributor

Fixes #12768

This patch addresses an issue where the client name will be printed as
what the "base / root" command was when invoking the "version" cmd.

Before

$ sudo ln -S $(which oc) ./newoc
$ ./newoc version
newoc v1.5.0-alpha.1+f3cb48a-659-dirty
kubernetes v1.5.2+43a9be4
features: Basic-Auth

Server https://10.13.137.149:8443
openshift v1.5.0-alpha.1+d776328-602-dirty
kubernetes v1.5.2+43a9be4

After

$ sudo ln -S $(which oc) ./newoc
$ ./newoc version
oc v1.5.0-alpha.1+f3cb48a-659-dirty
kubernetes v1.5.2+43a9be4
features: Basic-Auth

Server https://10.13.137.149:8443
openshift v1.5.0-alpha.1+d776328-602-dirty
kubernetes v1.5.2+43a9be4

cc @soltysh @openshift/cli-review

@juanvallejo juanvallejo changed the title don't use baseCmdName when printing client version don't use base cmd name when printing client version Feb 2, 2017
@juanvallejo
Copy link
Contributor Author

[test]

@juanvallejo juanvallejo mentioned this pull request Feb 2, 2017
Closed
@smarterclayton
Copy link
Contributor

The fix needs to be to not allow random names to be used for oc.

smarterclayton
smarterclayton reviewed Feb 3, 2017
View reviewed changes
pkg/cmd/cli/cmd/version.go Outdated
@@ -93,7 +93,7 @@ func (o *VersionOptions) Complete(cmd *cobra.Command, f *clientcmd.Factory, out

// RunVersion attempts to display client and server versions for Kubernetes and OpenShift
func (o VersionOptions) RunVersion() error {
fmt.Fprintf(o.Out, "%s %v\n", o.BaseName, version.Get())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not the right fix, this breaks versions for all the commands that reuse this code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider replacing basename as "oc" in cli.go:295

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@louyihua Thanks for the suggestion, I have updated the PR

@juanvallejo juanvallejo force-pushed the jvallejo/dont-use-basecmdname-oc-version branch from cdf475c to cd7b290 Compare February 10, 2017 19:01
@juanvallejo
Copy link
Contributor Author

check_future flaked on #12911 re[test]

@juanvallejo juanvallejo force-pushed the jvallejo/dont-use-basecmdname-oc-version branch from cd7b290 to 59e9093 Compare February 13, 2017 15:29
soltysh
soltysh reviewed Feb 20, 2017
View reviewed changes
pkg/cmd/cli/cli.go
@@ -292,7 +292,7 @@ func CommandFor(basename string) *cobra.Command {
case "kubectl":
cmd = NewCmdKubectl(basename, out)
default:
cmd = NewCommandCLI(basename, basename, in, out, errout)
cmd = NewCommandCLI("oc", "oc", in, out, errout)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is still not the right approach. This path is executed by all commands that are build from our repo, this includes oc, openshift, oadm. Make sure to check them all.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're not right. This is the default entry for the oc series command line utils (currently, oc only). The entries used by openshift and oadm series are not here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, you're right, sorry.

@fabianofranz
Copy link
Member

[merge]

@juanvallejo
don't use baseCmdName when printing client version
908676d 
This patch addresses an issue where the client name will be printed as
what the "base / root" command was when invoking the "version" cmd.
@juanvallejo juanvallejo force-pushed the jvallejo/dont-use-basecmdname-oc-version branch from 59e9093 to 908676d Compare March 10, 2017 15:16
@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 908676d

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/108/) (Base Commit: 8ca7c7b)

@fabianofranz
Copy link
Member

[merge]

@fabianofranz
Copy link
Member

flaked on #8571 re[merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 908676d

@openshift-bot
Copy link
Contributor

openshift-bot commented Mar 13, 2017 •
edited
Loading

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/62/) (Base Commit: 18b1328) (Image: devenv-rhel7_6067)

@openshift-bot openshift-bot merged commit 9e4e2e7 into openshift:master Mar 13, 2017
@juanvallejo juanvallejo deleted the jvallejo/dont-use-basecmdname-oc-version branch March 13, 2017 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@soltysh soltysh soltysh left review comments

@smarterclayton smarterclayton smarterclayton left review comments

@louyihua louyihua louyihua left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@juanvallejo @smarterclayton @fabianofranz @openshift-bot @soltysh @louyihua