remove openshift infra command by deads2k · Pull Request #17482 · openshift/origin

deads2k · 2017-11-27T21:13:06Z

This removes the openshift infra command. It does not replace

-		irouter.NewCommandTemplateRouter("router"),		
-		irouter.NewCommandF5Router("f5-router"),		
-		deployer.NewCommandDeployer("deploy"),		
-		recycle.NewCommandRecycle("recycle", out),		
-		builder.NewCommandS2IBuilder("sti-build"),		
-		builder.NewCommandDockerBuilder("docker-build"),

since those have their own binaries.

@sosiouxme I created a new command openshift-diagnostics to hold openshift-diagnostics diagnostic-pod and openshift-diagnostics network-diagnostic-pod. I'm not attached to the name, but looking at the dependency tree, I'm betting these move with oc later this month.

@smarterclayton I think this fulfills the intent of our conversation last week.
@juanvallejo fyi

bparees · 2017-11-27T21:17:12Z

/unassign

smarterclayton · 2017-11-27T21:23:24Z

hack/lib/constants.sh

  cmd/openshift
  cmd/oc
  cmd/kubefed
+  cmd/openshift-diagnostics


Hrm, confusing that this is just the binary used by diagnostics in pods. It’s fine for now.

Hrm, confusing that this is just the binary used by diagnostics in pods. It’s fine for now.

Yeah. I'm still open to different ways of structuring it as we push through 3.9. For now, I need them out of openshift so I can snip kubectl dependencies and they likely belong together under a related suite of commands.

@deads2k @smarterclayton got no problem with breaking this out, and don't much care about the name, but it needs to actually be included in the image which it isn't in 3.9-alpha.0... how can I make that happen?

@deads2k @smarterclayton got no problem with breaking this out, and don't much care about the name, but it needs to actually be included in the image which it isn't in 3.9-alpha.0... how can I make that happen?

I would try to work backwards from images/origin/Dockerfile. We're installing the RPM, so if it isn't present, I suspect the RPM doesn't include it, which probably means fixing up origin.spec.

smarterclayton · 2017-11-27T21:23:50Z

Lgtm

mfojtik · 2017-11-28T09:20:31Z

cmd/openshift-diagnostics/main.go

+	_ "k8s.io/kubernetes/pkg/api/install"
+	_ "k8s.io/kubernetes/pkg/apis/autoscaling/install"
+	_ "k8s.io/kubernetes/pkg/apis/batch/install"
+	_ "k8s.io/kubernetes/pkg/apis/extensions/install"


do we need apps ?

cmd/openshift-diagnostics/main.go

No, the main one installs all. We are in need of an overhaul for installation though.

mfojtik · 2017-11-28T09:21:21Z

/lgtm

need update of complete

openshift-merge-robot · 2017-11-28T09:21:39Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~OWNERS~~ [deads2k,mfojtik]

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

openshift-bot · 2017-11-28T11:47:57Z

/retest

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

deads2k · 2017-11-28T13:55:25Z

/retest

sosiouxme · 2017-11-28T14:18:29Z

Cool, thanks for the heads up. Is there a card or doc summarizing the intent of the changes?

deads2k · 2017-11-28T14:40:37Z

Cool, thanks for the heads up. Is there a card or doc summarizing the intent of the changes?

It's related to this: https://trello.com/c/xemjduCn/1068-21-move-oc-to-external-repository

We need to split oc out of origin and we're chasing down everything with a dependency from openshift on oc and this fell out.

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

/retest

openshift-merge-robot · 2017-11-28T20:17:00Z

Automatic merge from submit-queue (batch tested with PRs 17299, 17482, 17471).

@sosiouxme

Automatic merge from submit-queue (batch tested with PRs 17299, 17482, 17471). remove openshift infra command This removes the `openshift infra` command. It does not replace ``` - irouter.NewCommandTemplateRouter("router"), - irouter.NewCommandF5Router("f5-router"), - deployer.NewCommandDeployer("deploy"), - recycle.NewCommandRecycle("recycle", out), - builder.NewCommandS2IBuilder("sti-build"), - builder.NewCommandDockerBuilder("docker-build"), ``` since those have their own binaries. @sosiouxme I created a new command `openshift-diagnostics` to hold `openshift-diagnostics diagnostic-pod` and `openshift-diagnostics network-diagnostic-pod`. I'm not attached to the name, but looking at the dependency tree, I'm betting these move with `oc` later this month. @smarterclayton I think this fulfills the intent of our conversation last week. @juanvallejo fyi

@deads2k

…g-oc Automatic merge from submit-queue. move pkg/cmd/util/clientcmd -> pkg/oc/cli/util/clientcmd This patch *partially* solves a few of the items (currently checked) from #17309 Now that `clientcmd` (which includes printer factory methods) is moved into `pkg/oc`, the following files outside of `pkg/oc` need to have their dependency on `clientcmd` broken (this will be done in a separate PR): - [x] pkg/cmd/server/origin/controller/config.go (#17357) - [x] pkg/cmd/server/admin/create_error_template.go (#17357) - [x] pkg/cmd/server/admin/create_bootstrap_project_template.go (#17357) - [x] pkg/cmd/server/admin/overwrite_bootstrappolicy.go (*this file has been removed by* #17336) - [x] pkg/cmd/server/admin/create_login_template.go (#17357) - [x] pkg/cmd/server/admin/create_provider_selection_template.go (#17357) - [x] **pkg/cmd/infra/router/template.go** (Wanted by: `pkg/cmd/openshift/openshift.go` (no other dependents)) (#17357) - [x] **pkg/cmd/infra/router/f5.go** (Wanted by: `pkg/cmd/openshift/openshift.go` (no other dependents)) (#17357) - [x] pkg/cmd/openshift/openshift.go (#17486 and #17482) - [x] pkg/cmd/dockerregistry/dockerregistry.go (Wanted by: `cmd/dockerregistry/main.go` (depends on `clientcmd.Config`)) (#17357) - [x] *pkg/diagnostics/networkpod/util/util.go* (Can be addressed by moving `pkg/diagnostics` inside existing `pkg/oc/admin/diagnostics`) #17393 - [x] *pkg/diagnostics/client/config_contexts.go* (Can be addressed by moving `pkg/diagnostics` inside existing `pkg/oc/admin/diagnostics`) #17393 - [x] *pkg/diagnostics/client/run_diagnostics_pod.go* (Can be addressed by moving `pkg/diagnostics` inside existing `pkg/oc/admin/diagnostics`) #17393 - [x] *pkg/diagnostics/pod/auth.go* (Can be addressed by moving `pkg/diagnostics` inside existing `pkg/oc/admin/diagnostics`) #17393 - [x] *pkg/diagnostics/network/run_pod.go* (Can be addressed by moving `pkg/diagnostics` inside existing `pkg/oc/admin/diagnostics`) #17393 - [x] pkg/ipfailover/keepalived/plugin.go (moved to `pkg/oc/experimental`) (#17357) - [x] pkg/federation/kubefed/kubefed.go (#17357) - [x] pkg/dockerregistry/server/client/client.go (#17357) - [x] pkg/dockerregistry/server/auth_test.go (#17357) **bold** = depends on `clientcmd.Config` (not sure what to do about this) AND only dependent is `pkg/cmd/openshift/openshift.go` cc @deads2k @openshift/cli-review @liggitt

- openshift#17482 removed 'openshift infra' command and diagnostic-pod/network-diagnostic-pod subcommands now are moved under openshift-diagnostics command.

sosiouxme · 2017-12-15T15:55:22Z

On Fri, Dec 15, 2017 at 10:39 AM, David Eads ***@***.***> wrote: I would try to work backwards from images/origin/Dockerfile. We're installing the RPM, so if it isn't present, I suspect the RPM doesn't include it, which probably means fixing up origin.spec.

Thanks, was looking at it yesterday and that was where I was headed. It's a 162M binary; I was wondering if that was OK to add to the RPM (already kind of huge) or if it should be folded back into an existing binary and packaged with a symlink.

deads2k · 2017-12-15T18:16:28Z

Thanks, was looking at it yesterday and that was where I was headed. It's
a 162M binary; I was wondering if that was OK to add to the RPM (already
kind of huge) or if it should be folded back into an existing binary and
packaged with a symlink.

It was split out because the openshift binary cannot have any dependency on k8s.io/kubernetes/pkg/kubectl. That means it cannot be included in the openshift command. Also, having single binaries that do different things based on name seems highly suspect.

Automatic merge from submit-queue. Install openshift-diagnostics binary in dind and vagrant environments - #17482 removed 'openshift infra' command and diagnostic-pod/network-diagnostic-pod subcommands now are moved under openshift-diagnostics command.

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 27, 2017

openshift-merge-robot assigned bparees and mfojtik Nov 27, 2017

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

openshift-ci-robot unassigned bparees Nov 27, 2017

smarterclayton reviewed Nov 27, 2017

View reviewed changes

This was referenced Nov 27, 2017

move pkg/cmd/util/clientcmd -> pkg/oc/cli/util/clientcmd #17356

Merged

start breaking deps on pkg/oc #17309

Closed

mfojtik reviewed Nov 28, 2017

View reviewed changes

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

deads2k added 2 commits November 28, 2017 07:53

remove openshift infra command

28f8685

generated

0a63dd8

deads2k force-pushed the bin-09-infra branch from d3d3f68 to 0a63dd8 Compare November 28, 2017 12:58

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 28, 2017

openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 28, 2017

deads2k added the lgtm Indicates that a PR is ready to be merged. label Nov 28, 2017

openshift-merge-robot merged commit e776aff into openshift:master Nov 28, 2017

pravisankar mentioned this pull request Dec 14, 2017

Install openshift-diagnostics binary in dind and vagrant environments #17816

Merged

deads2k deleted the bin-09-infra branch January 24, 2018 14:37

Conversation

deads2k commented Nov 27, 2017

Uh oh!

bparees commented Nov 27, 2017

Uh oh!

smarterclayton Nov 27, 2017

Choose a reason for hiding this comment

Uh oh!

deads2k Nov 27, 2017

Choose a reason for hiding this comment

Uh oh!

sosiouxme Dec 13, 2017

Choose a reason for hiding this comment

Uh oh!

deads2k Dec 15, 2017

Choose a reason for hiding this comment

Uh oh!

smarterclayton commented Nov 27, 2017

Uh oh!

mfojtik Nov 28, 2017

Choose a reason for hiding this comment

Uh oh!

deads2k Nov 28, 2017

Choose a reason for hiding this comment

Uh oh!

mfojtik commented Nov 28, 2017

Uh oh!

openshift-merge-robot commented Nov 28, 2017

Uh oh!

openshift-bot commented Nov 28, 2017

Uh oh!

deads2k commented Nov 28, 2017

Uh oh!

sosiouxme commented Nov 28, 2017

Uh oh!

deads2k commented Nov 28, 2017

Uh oh!

deads2k commented Nov 28, 2017

Uh oh!

openshift-merge-robot commented Nov 28, 2017

Uh oh!

sosiouxme commented Dec 15, 2017 via email

Uh oh!

deads2k commented Dec 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants