break dependency on version cmd for non-cli pkgs by juanvallejo · Pull Request #17335 · openshift/origin

juanvallejo · 2017-11-15T23:08:34Z

This patch solves a few of the items (currently checked) from #17309
Removes dependency on pkg/cli/cmd/version for packages outside
of the pkg/oc subtree.

cc @deads2k @liggitt @openshift/cli-review

deads2k · 2017-11-16T14:16:56Z

pkg/cmd/version/version.go

+		Long:  "Display client and server versions",
+		Run: func(cmd *cobra.Command, args []string) {
+			fmt.Fprintf(out, "%s %v\n", fullName, version.Get())
+			fmt.Fprintf(out, "kubernetes %v\n", kubeversion.Get())


These commands shouldn't be showing a kube or an etcd version

deads2k · 2017-11-16T14:18:03Z

pkg/cmd/version/version.go

+
+	kubeversion "k8s.io/kubernetes/pkg/version"
+
+	"github.com/openshift/origin/pkg/version"


This dependency is incorrect as we start splitting these out of tree. Update it to take a version interface and force the caller to provide it. Take inspiration from the version endpoint in k8s.io/apiserver, but do not directly reference it.

deads2k · 2017-11-16T14:29:48Z

@juanvallejo see how badly this conflict with #17115

juanvallejo · 2017-11-16T21:53:27Z

@deads2k

see how badly this conflict with #17115

Added a new commit that receives a VersionInfo, forcing callers of pkg/cmd/version#NewCmdVersion to call it with a provider of version info.

Saw no conflict with rebase branch locally.

deads2k · 2017-11-17T15:54:31Z

pkg/cmd/server/start/kubernetes/kubernetes.go

 	cmds.AddCommand(NewSchedulerCommand("scheduler", fullName+" scheduler", out))
 	if "hyperkube" == fullName {
-		cmds.AddCommand(cmd.NewCmdVersion(fullName, nil, out, cmd.VersionOptions{}))
+		cmds.AddCommand(cmdversion.NewCmdVersion(fullName, version.Get(), out))


Not this one. This one is just wrong. It is a legitimate target for removal.

deads2k · 2017-11-17T15:54:52Z

pkg/cmd/version/version.go

+)
+
+// VersionInfo provides semantic version information
+// in a humnan-friendly format


deads2k · 2017-11-17T15:57:32Z

pkg/cmd/version/version.go

+// VersionInfo provides semantic version information
+// in a humnan-friendly format
+type VersionInfo interface {
+	String() string


TODO, may be expanded for various short and formatting options if necessary.

deads2k · 2017-11-17T15:58:09Z

pkg/federation/kubefed/kubefed.go

 	// Use the openshift-specific version command
-	cmds.AddCommand(cmd.NewCmdVersion("kubefed", f, out, cmd.VersionOptions{PrintClientFeatures: true}))
-
+	cmds.AddCommand(versioncmd.NewCmdVersion("kubefed", version.Get(), out))


not this one for now either. It also looks wrong.

deads2k · 2017-11-17T15:59:23Z

minor comments, then squash.

@juanvallejo for future reference, if I need separate commits for re-review, I'll specifically mention it in my review. I think sttts always prefers separate commits. Just a personal preference to keep things rolling smoothly.

juanvallejo · 2017-11-17T16:25:20Z

@deads2k thanks for the review, comments addressed, commits squashed

juanvallejo · 2017-11-17T19:16:07Z

/retest

deads2k · 2017-11-17T19:22:53Z

/lgtm

See comment above about not conflicting with the rebase.

openshift-merge-robot · 2017-11-17T19:23:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, juanvallejo

The full list of commands accepted by this bot can be found here.

Details

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

~~pkg/OWNERS~~ [deads2k]

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

juanvallejo · 2017-11-17T21:48:07Z

/retest

openshift-bot · 2017-11-18T15:52:28Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-merge-robot · 2017-11-18T22:05:14Z

Automatic merge from submit-queue.

openshift-ci-robot · 2017-11-18T23:01:52Z

@juanvallejo: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/extended_builds	`79b3fbe`	link	`/test extended_builds`
ci/openshift-jenkins/extended_conformance_install_update	`79b3fbe`	link	`/test extended_conformance_install_update`
ci/openshift-jenkins/extended_conformance_gce	`79b3fbe`	link	`/test extended_conformance_gce`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

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. I understand the commands that are listed here.

openshift-merge-robot assigned tiran and 0xmichalis Nov 15, 2017

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 15, 2017

juanvallejo mentioned this pull request Nov 15, 2017

start breaking deps on pkg/oc #17309

Closed

19 tasks

juanvallejo force-pushed the jvallejo/break-cmd-version-dependency-non-cli-pkgs branch from 9319d84 to 3ef4a4b Compare November 15, 2017 23:25

deads2k reviewed Nov 16, 2017

View reviewed changes

juanvallejo force-pushed the jvallejo/break-cmd-version-dependency-non-cli-pkgs branch 2 times, most recently from 319ca13 to 739b21b Compare November 16, 2017 21:59

deads2k reviewed Nov 17, 2017

View reviewed changes

break dependency on version cmd for non-cli pkgs

79b3fbe

juanvallejo force-pushed the jvallejo/break-cmd-version-dependency-non-cli-pkgs branch from 739b21b to 79b3fbe Compare November 17, 2017 16:25

0xmichalis assigned deads2k and unassigned tiran and 0xmichalis Nov 17, 2017

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 17, 2017

openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 17, 2017

openshift-merge-robot merged commit 8ffa313 into openshift:master Nov 18, 2017

juanvallejo deleted the jvallejo/break-cmd-version-dependency-non-cli-pkgs branch November 20, 2017 16:00


		kubeversion "k8s.io/kubernetes/pkg/version"

		"github.com/openshift/origin/pkg/version"

Conversation

juanvallejo commented Nov 15, 2017

Uh oh!

deads2k Nov 16, 2017

Choose a reason for hiding this comment

Uh oh!

deads2k Nov 16, 2017

Choose a reason for hiding this comment

Uh oh!

deads2k commented Nov 16, 2017

Uh oh!

juanvallejo commented Nov 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deads2k Nov 17, 2017

Choose a reason for hiding this comment

Uh oh!

deads2k Nov 17, 2017

Choose a reason for hiding this comment

Uh oh!

deads2k Nov 17, 2017

Choose a reason for hiding this comment

Uh oh!

deads2k Nov 17, 2017

Choose a reason for hiding this comment

Uh oh!

deads2k commented Nov 17, 2017

Uh oh!

juanvallejo commented Nov 17, 2017

Uh oh!

juanvallejo commented Nov 17, 2017

Uh oh!

deads2k commented Nov 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-merge-robot commented Nov 17, 2017

Uh oh!

juanvallejo commented Nov 17, 2017

Uh oh!

openshift-bot commented Nov 18, 2017

Uh oh!

openshift-merge-robot commented Nov 18, 2017

Uh oh!

openshift-ci-robot commented Nov 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

juanvallejo commented Nov 16, 2017 •

edited

Loading

deads2k commented Nov 17, 2017 •

edited

Loading