cli: remove usage of internal apps helpers from all CLI commands #20362

mfojtik · 2018-07-19T12:58:47Z

This PR removes usage of appsinternalutil from all CLI commands, making the CLI not depend on the internal apps types.

/cc @juanvallejo
/cc @deads2k
/cc @tnozicka

mfojtik · 2018-07-19T13:00:36Z

pkg/apps/util/const.go

+
+	// deploymentStatusReasonAnnotation represents the reason for deployment being in a given state
+	// Used for specifying the reason for cancellation or failure of a deployment
+	deploymentStatusReasonAnnotation = "openshift.io/deployment.status-reason"


@tnozicka note that this collection is used exclusively by util helpers and nothing else... i thinks we should not make it public but have helpers that allow to use them.

I'd make it public and moved it to library-go or somewhere so people can import it. The annotation is API-ish as that's part of the public facing status and an annotation just because we don't control the API for the upstream object.

i thinks we should not make it public but have helpers that allow to use them.

I thought we are trying to get rid of those one line helpers.

Those are useful consts for someone writing an integration, making them private will just lead to code duplication

mfojtik · 2018-07-19T13:01:02Z

pkg/apps/util/deployment.go

+
+	deploymentName := LatestDeploymentNameForConfig(config)
+
+	podSpec := v1.PodSpec{}


err have to drop this

mfojtik · 2018-07-19T13:24:31Z

pkg/oc/cli/admin/prune/deployments/resolvers.go

@@ -86,6 +86,15 @@ func NewPerDeploymentConfigResolver(dataSet DataSet, keepComplete int, keepFaile
 	}
 }

+// ByMostRecent sorts deployments by most recently created.
+type ByMostRecent []*kapi.ReplicationController


only resolvers use this, I don't see a reason for this to live in apps util package... if resolvers want special sort, then lets make them define it here.

mfojtik · 2018-07-19T13:25:29Z

pkg/oc/cli/rollback/rollback.go

-	kc kclientset.Interface
+
+	appsClient appstypedclient.AppsV1Interface
+	kubeClient kubernetes.Interface


@juanvallejo @deads2k there are some commands I had to switch to external client now to avoid local conversions

mfojtik · 2018-07-19T13:25:56Z

pkg/oc/cli/rollback/rollback.go

-		configName = appsinternalutil.DeploymentConfigNameFor(obj)
-	case *appsapi.DeploymentConfig:
+		configName = appsutil.DeploymentConfigNameFor(obj)
+	case *appsv1.DeploymentConfig:


@deads2k @juanvallejo can you confirm this will work? :-)

I think you have to change the scheme in the resource builder in o.findResource to be WithScheme(ocscheme.ReadingInternalScheme, ocscheme.ReadingInternalScheme.PrioritizedVersionsAllGroups()...), otherwise, you'll still be dealing with internal objects here

fixed, thanks!

mfojtik · 2018-07-19T13:26:57Z

pkg/oc/lib/describe/deployments_test.go

-				*appsapitest.OkHPAForDeploymentConfig(config, 1, 3),
-			}}, nil
-	})
+	// TODO: re-enable when we switch describer to external client


every big win has some small looses :-)

@juanvallejo I can deal with this in a follow up PR, but I didn't want to groom this PR with switching the HPA to external as well (+300 lines of changes)

mfojtik · 2018-07-19T13:27:42Z

pkg/oc/lib/describe/describer.go

@@ -77,7 +82,7 @@ func describerMap(clientConfig *rest.Config, kclient kclientset.Interface, host
 	if err != nil {
 		glog.V(1).Info(err)
 	}
-	appsClient, err := appsclient.NewForConfig(clientConfig)
+	appsClient, err := appstypedclient.NewForConfig(clientConfig)


@soltysh @juanvallejo i believe this is the holy grail path and we want to switch all describers to external at some point?

hm, what about upstream describers? I believe some of those make use of the pkg/printers/internalversion package.
Since we do not use the printers/internalversion package in kubectl to the extent that we used to (with humanreadable printing) due to the introduction of server-side printing, maybe the best path forward for describers would be something similar to server-side printing?

@juanvallejo not super familiar with server side printing, but in general I think describers are not that generic and are very custom for some resources. I think for origin resources, as we continue moving forward with external clients and external types, we will likely convert our describers to external... I would expect that if kubectl is also taking the external path, they will do the same...

/cc @soltysh

At least in the scope of this PR, does switching the describer func for deploymentconfigs affect the oc describe command?

mfojtik · 2018-07-19T13:29:51Z

pkg/apps/controller/util/util.go

@@ -74,12 +75,6 @@ func DecodeDeploymentConfig(controller metav1.ObjectMetaAccessor) (*appsapi.Depl
 	return config, nil
 }

-// DeployerPodSelector returns a label Selector which can be used to find all


@deads2k you asked about number of remaining internal helpers... these are left overs (used by REST and controllers)... Note that not all if them are duplicated into pkg/apps/util, some live sorely to make just controllers happy.

@deads2k you asked about number of remaining internal helpers... these are left overs (used by REST and controllers)... Note that not all if them are duplicated into pkg/apps/util, some live sorely to make just controllers happy.

glad to see it moving in the right direction

mfojtik · 2018-07-19T14:13:18Z

/retest

juanvallejo · 2018-07-19T14:44:32Z

pkg/oc/cli/rollback/rollback.go

-	if o.kc == nil {
-		return fmt.Errorf("kc must not be nil")
+	if o.kubeClient == nil {
+		return fmt.Errorf("kubeInternalClient must not be nil")
 	}
 	if o.getBuilder == nil {
 		return fmt.Errorf("getBuilder must not be nil")


I don't think this validation check is necessary - we don't do it in any other commands

yeah, when I saw it I thought it is weird... will remove that

mfojtik · 2018-07-20T10:14:03Z

/retest

mfojtik · 2018-07-20T11:40:12Z

/retest

mfojtik · 2018-07-23T16:26:36Z

/retest

mfojtik · 2018-07-23T17:11:52Z

/retest

juanvallejo · 2018-07-23T21:21:48Z

pkg/oc/lib/describe/helpers.go

@@ -54,7 +55,16 @@ func bold(v interface{}) string {
 	return "\033[1m" + toString(v) + "\033[0m"
 }

-func convertEnv(env []api.EnvVar) map[string]string {
+// DEPRECATED:
+func convertEnvInternal(env []api.EnvVar) map[string]string {


Is this still used by something? Do we need to keep it?

not used, I can remove it in follow up if you're fine with that (don't want to mess with the queue now)

mfojtik · 2018-07-24T11:06:25Z

/refresh

openshift-ci-robot · 2018-07-25T10:02:14Z

New changes are detected. LGTM label has been removed.

mfojtik · 2018-07-25T10:46:18Z

reapplying LGTM label, simple conflict fixed in oc rollout cancel.

mfojtik · 2018-07-25T11:03:25Z

/retest

mfojtik · 2018-07-25T14:02:32Z

/retest

mfojtik · 2018-07-25T14:21:14Z

/retest

openshift-ci-robot · 2018-07-26T08:12:46Z

New changes are detected. LGTM label has been removed.

mfojtik · 2018-07-26T08:13:00Z

re-adding lgtm, simple conflict in describer

mfojtik requested review from soltysh and deads2k July 19, 2018 12:58

openshift-ci-robot requested review from juanvallejo and tnozicka July 19, 2018 12:58

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 19, 2018

mfojtik commented Jul 19, 2018

View reviewed changes

mfojtik force-pushed the apps-04-refactor-cli branch from 2c7c016 to d8f94cb Compare July 19, 2018 13:03

mfojtik mentioned this pull request Jul 19, 2018

switch logs, idle commands to externals #20343

Merged

mfojtik commented Jul 19, 2018

View reviewed changes

mfojtik force-pushed the apps-04-refactor-cli branch from e793ff9 to d2430e1 Compare July 19, 2018 13:31

mfojtik force-pushed the apps-04-refactor-cli branch 4 times, most recently from e2b8017 to 80f1367 Compare July 19, 2018 14:44

juanvallejo reviewed Jul 19, 2018

View reviewed changes

mfojtik force-pushed the apps-04-refactor-cli branch from 80f1367 to 0f18c67 Compare July 19, 2018 15:07

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 19, 2018

mfojtik force-pushed the apps-04-refactor-cli branch from 0f18c67 to 1ca8f04 Compare July 20, 2018 08:35

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 20, 2018

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 20, 2018

juanvallejo reviewed Jul 23, 2018

View reviewed changes

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 24, 2018

mfojtik force-pushed the apps-04-refactor-cli branch from 2208e68 to fc7d8d2 Compare July 25, 2018 10:02

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 25, 2018

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 25, 2018

mfojtik added the lgtm Indicates that a PR is ready to be merged. label Jul 25, 2018

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 25, 2018

mfojtik added 7 commits July 26, 2018 10:12

apps: re-introduce versioned MakeDeployment and add test sub-package

5bac1b1

apps: make rollout log client external

6beb022

apps: convert more utils

7c25952

apps: rename ok test to internaltest

d0de7f1

cli: convert apps commands to external helpers

4ae189a

apps: remove more internal helpers

c6b1a23

apps: move deployer controller to external types

610fb48

mfojtik force-pushed the apps-04-refactor-cli branch from fc7d8d2 to 610fb48 Compare July 26, 2018 08:12

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 26, 2018

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 26, 2018

mfojtik added the lgtm Indicates that a PR is ready to be merged. label Jul 26, 2018

openshift-merge-robot merged commit 305c0c2 into openshift:master Jul 26, 2018

mfojtik deleted the apps-04-refactor-cli branch September 5, 2018 21:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cli: remove usage of internal apps helpers from all CLI commands #20362

cli: remove usage of internal apps helpers from all CLI commands #20362

mfojtik commented Jul 19, 2018

mfojtik Jul 19, 2018

tnozicka Jul 23, 2018

tnozicka Jul 23, 2018

mfojtik Jul 19, 2018

mfojtik Jul 19, 2018

mfojtik Jul 19, 2018

mfojtik Jul 19, 2018

juanvallejo Jul 19, 2018

mfojtik Jul 20, 2018

mfojtik Jul 19, 2018

mfojtik Jul 19, 2018

juanvallejo Jul 19, 2018

mfojtik Jul 20, 2018

juanvallejo Jul 23, 2018

mfojtik Jul 19, 2018

deads2k Jul 19, 2018

mfojtik commented Jul 19, 2018

juanvallejo Jul 19, 2018

mfojtik Jul 19, 2018

mfojtik commented Jul 20, 2018

mfojtik commented Jul 20, 2018

mfojtik commented Jul 23, 2018

mfojtik commented Jul 23, 2018

juanvallejo Jul 23, 2018

mfojtik Jul 24, 2018

mfojtik commented Jul 24, 2018

openshift-ci-robot commented Jul 25, 2018

mfojtik commented Jul 25, 2018

mfojtik commented Jul 25, 2018

mfojtik commented Jul 25, 2018

mfojtik commented Jul 25, 2018

openshift-ci-robot commented Jul 26, 2018

mfojtik commented Jul 26, 2018


		deploymentName := LatestDeploymentNameForConfig(config)

		podSpec := v1.PodSpec{}

cli: remove usage of internal apps helpers from all CLI commands #20362

cli: remove usage of internal apps helpers from all CLI commands #20362

Conversation

mfojtik commented Jul 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfojtik commented Jul 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfojtik commented Jul 20, 2018

mfojtik commented Jul 20, 2018

mfojtik commented Jul 23, 2018

mfojtik commented Jul 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfojtik commented Jul 24, 2018

openshift-ci-robot commented Jul 25, 2018

mfojtik commented Jul 25, 2018

mfojtik commented Jul 25, 2018

mfojtik commented Jul 25, 2018

mfojtik commented Jul 25, 2018

openshift-ci-robot commented Jul 26, 2018

mfojtik commented Jul 26, 2018