start breaking deps on pkg/oc #17309

juanvallejo · 2017-11-14T19:37:15Z

Begin breaking dependencies on the pkg/oc subtree from the rest of the codebase.
https://docs.google.com/document/d/1mS-KRkU8zjCleBNi3WWhLtkH10hb8h4ajpFM743jnqg

This PR only serves to track affected files and their respective PRs.
The following is a list of files whose dependency on pkg/oc packages needs to be broken:

bold = only depend on pkg/oc for the NewCmdVersion logic - will make simple version helper outside of pkg/oc to resolve

cc @liggitt @deads2k @mfojtik @bparees

openshift-merge-robot · 2017-11-14T19:37:32Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: juanvallejo
We suggest the following additional approver: smarterclayton

Assign the PR to them by writing /assign @smarterclayton in a comment when ready.

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

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

pkg/generate/OWNERS
pkg/oc/OWNERS

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

deads2k · 2017-11-14T19:39:28Z

@juanvallejo can you dig into this one: pkg/cmd/infra/builder/builder.go ? I'm betting its a function for going from kubeconfig file to clientConfig. the image-registry hit a similar problem

juanvallejo · 2017-11-14T19:42:16Z

@deads2k sure, will look into it now

juanvallejo · 2017-11-14T20:16:33Z

@deads2k The dependency is only on NewCmdVersion. pkg/cmd/infra/builder/builder.go defines command entry points for:

$ openshift-sti-build
$ openshift-docker-build
$ openshift-git-clone
$ openshift-manage-dockerfile
$ openshift-extract-image-content

The same is true for pkg/cmd/infra/router/f5.go, pkg/cmd/infra/deployer/deployer.go, pkg/cmd/infra/router/template.go. and pkg/cmd/server/start/kubernetes/kubernetes.go

$ openshift-f5-router
$ openshift-deploy
$ openshift-router

Any chance we could just make all of these subcommands under oc?
The alternative would be to move or duplicate the code under pkg/oc/cli/cmd/version.go, or to remove the version subcommand from the above.

deads2k · 2017-11-14T20:58:07Z

pkg/generate/app/cmd/ says oc to me. Who uses it?

juanvallejo · 2017-11-14T21:04:03Z

@deads2k pkg/generate/app/cmd/describe.go was the only file under that subtree I found with a reference to pkg/oc - was using a time formatter in the pkg/oc/cli/describe/helpers.go. Addressed in a5e30bb

deads2k · 2017-11-14T21:12:19Z

@deads2k pkg/generate/app/cmd/describe.go was the only file under that subtree I found with a reference to pkg/oc - was using a time formatter in the pkg/oc/cli/describe/helpers.go. Addressed in a5e30bb

@juanvallejo pkg/generate/app/cmd is only imported by packages under pkg/oc. Move it under pkg/oc

juanvallejo · 2017-11-14T21:43:18Z

pkg/cmd/util/clientcmd/factory_object_mapping.go -> depends on the oc describe#DescriberFor method (this is the only time it is called in the entire pkg/cmd/util/clientcmd pkg)
- @deads2k would it make sense to move pkg/oc/cli/describe/describer.go outside of pkg/oc? (the file itself has no references to any packages within the pkg/oc subtree)
pkg/cmd/util/clientcmd/factory_client_access.go -> depends on the oc describe#NewHumanReadablePrinter method AND pkg/oc/cli/config#NewOpenShiftClientConfigLoadingRules method
- To break dependency on pkg/oc/cli/config move NewOpenShiftClientConfigLoadingRules() outside of pkg/oc
- Set the config flag elsewhere: https://github.com/openshift/origin/blob/master/pkg/cmd/util/clientcmd/factory_client_access.go#L111

juanvallejo · 2017-11-14T22:14:01Z

pkg/cmd/server/start/master_args.go depends on generated bootstrap bindata, which is under pkg/oc/bootstrap - not sure how we plan to address this

juanvallejo · 2017-11-14T22:30:20Z

pkg/cmd/server/admin/overwrite_bootstrappolicy.go depends on the cli describer. Apparently it defines the oc adm overwrite-policy command, but seems to live under pkg/cmd/server since it deals with authorization storage. Can this be moved under pkg/oc/admin?

sosiouxme · 2017-11-15T19:29:58Z

@juanvallejo what would it mean to "move some of the diagnostic commands under pkg/oc"? I don't think there's a reason to have oc adm diagnostics command definitions live outside pkg/oc/admin.

@deads2k

…-dependency-non-cli-pkgs Automatic merge from submit-queue. break dependency on version cmd for non-cli pkgs 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

…/modify_roles_test.go

openshift-ci-robot · 2017-11-21T00:01:10Z

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

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/extended_clusterup	`c295644`	link	`/test extended_clusterup`
ci/openshift-jenkins/extended_conformance_install	`c295644`	link	`/test extended_conformance_install`
ci/openshift-jenkins/extended_builds	`c295644`	link	`/test extended_builds`
ci/openshift-jenkins/extended_conformance_gce	`c295644`	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.

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.

@deads2k

…strap-policy Automatic merge from submit-queue. Remove overwrite_bootstrappolicy and pkg/cmd/server/admin/legacyetcd Fixes #15817 This patch solves a few of the items (currently checked) from #17309 Removes the file `pkg/cmd/server/admin/overwrite_bootstrappolicy.go` which has been deprecated (and had some dependencies on `pkg/oc`. It also removes the deprecated package `pkg/cmd/server/admin/legacyetcd`. cc @deads2k @liggitt @openshift/cli-review

juanvallejo · 2017-11-27T16:59:37Z

@deads2k only dependency left to address is pkg/cmd/openshift/openshift.go which brings in nine packages from pkg/oc

@liggitt

…pkg-oc Automatic merge from submit-queue. move pkg/diagnostices -> pkg/oc/admin/diagnostics This patch is a part of pull/17356 - it aims to break all dependencies between packages outside of pkg/oc and the clientcmd package. This patch also partially solves a few of the items (currently checked) from #17309. By moving `pkg/diagnostics` to the `pkg/oc` subtree, we are eliminating package dependencies on `pkg/oc/...` in packages outside of that subtree. cc @liggitt @deads2k @openshift/cli-review @sosiouxme

@deads2k

…o-pkg-oc Automatic merge from submit-queue (batch tested with PRs 17417, 17332). Begin moving pkgs w/ deps on pkg/oc This patch solves a few of the items (currently checked) from #17309 cc @deads2k @openshift/cli-review @liggitt

@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

@deads2k

…cmds-to-pkg-oc Automatic merge from submit-queue. move "openshift ex" -> "oc ex" Moves the experimental command group to `oc` in order to break deps on `pkg/oc` in packages outside of that subtree. Part of openshift#17309 and openshift#17356 Release-Note: `Experimental commands moved from the "openshift" parent to the "oc" parent` cc @deads2k @liggitt

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 14, 2017

openshift-merge-robot assigned jim-minter Nov 14, 2017

openshift-merge-robot assigned smarterclayton Nov 14, 2017

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 14, 2017

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 14, 2017

juanvallejo added 2 commits November 15, 2017 13:33

start breaking deps on pkg/oc

3de7b00

break dep on pkg/oc - generator/generator.go

6114358

juanvallejo force-pushed the jvallejo/break-outside-deps-on-oc-subtree branch from 4b9c4d9 to 7c5030a Compare November 15, 2017 18:33

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 15, 2017

juanvallejo mentioned this pull request Nov 20, 2017

move pkg/diagnostices -> pkg/oc/admin/diagnostics #17393

Merged

juanvallejo changed the title ~~[WIP] start breaking deps on pkg/oc~~ start breaking deps on pkg/oc Nov 20, 2017

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 20, 2017

juanvallejo added 2 commits November 20, 2017 16:37

move pkg/generate/app/cmd -> pkg/oc/genneratee/app/cmd

d1d236e

move pkg/cmd/admin/policy/modify_roles_test.go -> pkg/oc/admin/policy…

c295644

…/modify_roles_test.go

juanvallejo force-pushed the jvallejo/break-outside-deps-on-oc-subtree branch from 7c5030a to c295644 Compare November 20, 2017 21:38

juanvallejo mentioned this pull request Nov 27, 2017

move "openshift ex" -> "oc ex" #17486

Merged

juanvallejo closed this Nov 28, 2017

juanvallejo deleted the jvallejo/break-outside-deps-on-oc-subtree branch November 28, 2017 17:28

juanvallejo mentioned this pull request Dec 6, 2017

Tracking (even more) deps from origin on cli #17630

Closed

57 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

start breaking deps on pkg/oc #17309

start breaking deps on pkg/oc #17309

juanvallejo commented Nov 14, 2017 •

edited

Loading

openshift-merge-robot commented Nov 14, 2017

deads2k commented Nov 14, 2017

juanvallejo commented Nov 14, 2017

juanvallejo commented Nov 14, 2017 •

edited

Loading

deads2k commented Nov 14, 2017

juanvallejo commented Nov 14, 2017 •

edited

Loading

deads2k commented Nov 14, 2017

juanvallejo commented Nov 14, 2017 •

edited

Loading

juanvallejo commented Nov 14, 2017

juanvallejo commented Nov 14, 2017 •

edited

Loading

sosiouxme commented Nov 15, 2017

openshift-ci-robot commented Nov 21, 2017

juanvallejo commented Nov 27, 2017

start breaking deps on pkg/oc #17309

start breaking deps on pkg/oc #17309

Conversation

juanvallejo commented Nov 14, 2017 • edited Loading

openshift-merge-robot commented Nov 14, 2017

deads2k commented Nov 14, 2017

juanvallejo commented Nov 14, 2017

juanvallejo commented Nov 14, 2017 • edited Loading

deads2k commented Nov 14, 2017

juanvallejo commented Nov 14, 2017 • edited Loading

deads2k commented Nov 14, 2017

juanvallejo commented Nov 14, 2017 • edited Loading

juanvallejo commented Nov 14, 2017

juanvallejo commented Nov 14, 2017 • edited Loading

sosiouxme commented Nov 15, 2017

openshift-ci-robot commented Nov 21, 2017

juanvallejo commented Nov 27, 2017

juanvallejo commented Nov 14, 2017 •

edited

Loading

juanvallejo commented Nov 14, 2017 •

edited

Loading

juanvallejo commented Nov 14, 2017 •

edited

Loading

juanvallejo commented Nov 14, 2017 •

edited

Loading

juanvallejo commented Nov 14, 2017 •

edited

Loading