apps: switch to use generated internal clientset #16100

mfojtik · 2017-09-01T11:30:34Z

This PR replaces the legacy pkg/client in pkg/deploy. It switches controllers and storage to use the generated internal clientset.

mfojtik · 2017-09-01T11:31:53Z

pkg/cmd/server/origin/controller/interfaces.go

+	OpenshiftInternalTemplateClient(name string) (templateclient.Interface, error)
+	OpenshiftInternalTemplateClientOrDie(name string) templateclient.Interface
+
+	OpenshiftInternalImageClient(name string) (imageclientinternal.Interface, error)


@deads2k @soltysh minor, but I like this naming scheme better:

OpenShift[Internal|External]Client

It matches the kube client scheme and allow us to add external clients in future...

@deads2k @soltysh minor, but I like this naming scheme better:

OpenShift[Internal|External]Client

It matches the kube client scheme and allow us to add external clients in future...

ok

On a side not, I still wonder if we need both *Client and *ClientOrDie if we use the latter in all places that I've searched for.

@soltysh the apiserver use the *Client version to report errors properly, we need both.

mfojtik · 2017-09-01T11:33:22Z

pkg/deploy/apiserver/apiserver.go

@@ -102,7 +103,11 @@ func (c *AppsConfig) newV1RESTStorage() (map[string]rest.Storage, error) {
 	// TODO sort out who is using this and why.  it was hardcoded before the migration and I suspect that it is being used
 	// to serialize out objects into annotations.
 	externalVersionCodec := kapi.Codecs.LegacyCodec(schema.GroupVersion{Group: "", Version: "v1"})
-	deprecatedOpenshiftClient, err := osclient.New(c.GenericConfig.LoopbackClientConfig)
+	openshiftInternalAppsClient, err := appsclientinternal.NewForConfig(c.CoreAPIServerClientConfig)


@deads2k this almost looks like a job for ClientBuilder :-) (but I don't really think we should use the controller.ClientBuilder here, maybe just create something similar for GenericServer ?

@deads2k this almost looks like a job for ClientBuilder :-) (but I don't really think we should use the controller.ClientBuilder here, maybe just create something similar for GenericServer ?

The client builder is special because it goes and gets different credentials. I don't think this server needs to get different credentials to run

deads2k · 2017-09-01T11:36:46Z

pkg/deploy/apiserver/apiserver.go

@@ -102,7 +103,11 @@ func (c *AppsConfig) newV1RESTStorage() (map[string]rest.Storage, error) {
 	// TODO sort out who is using this and why.  it was hardcoded before the migration and I suspect that it is being used
 	// to serialize out objects into annotations.
 	externalVersionCodec := kapi.Codecs.LegacyCodec(schema.GroupVersion{Group: "", Version: "v1"})
-	deprecatedOpenshiftClient, err := osclient.New(c.GenericConfig.LoopbackClientConfig)
+	openshiftInternalAppsClient, err := appsclientinternal.NewForConfig(c.CoreAPIServerClientConfig)


This connection can be based on the loopback clientconfig (see previous). It doesn't have to go through the coreapiserver.

deads2k · 2017-09-01T11:37:19Z

pkg/deploy/apiserver/apiserver.go

+	if err != nil {
+		return nil, err
+	}
+	openshiftInternalImageClient, err := imageclientinternal.NewForConfig(c.CoreAPIServerClientConfig)


this one is using the correct clientconfig, since the apps server doesn't host images.

We should probably write it down for others to be aware of that rule, for the future time where we'll split each group into its own apiserver.

deads2k · 2017-09-01T11:39:18Z

lgtm so far

mfojtik · 2017-09-01T22:25:01Z

@deads2k @tnozicka PTAL - this is now ready, tests should be green.

0xmichalis · 2017-09-02T22:06:23Z

/lint

openshift-ci-robot

@Kargakis: 8 warnings.

In response to this:

/lint

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.

openshift-ci-robot · 2017-09-02T22:08:03Z

pkg/deploy/registry/instantiate/rest.go

 )

-func NewREST(store registry.Store, oc client.Interface, kc kclientset.Interface, decoder runtime.Decoder, admission admission.Interface) *REST {
+func NewREST(store registry.Store, imagesclient imageclientinternal.Interface, kc kclientset.Interface, decoder runtime.Decoder, admission admission.Interface) *REST {


Golint comments: exported function NewREST should have comment or be unexported. More info.

openshift-ci-robot · 2017-09-02T22:08:03Z

pkg/cmd/server/origin/controller/interfaces.go

@@ -99,32 +108,48 @@ func (b OpenshiftControllerClientBuilder) DeprecatedOpenshiftClientOrDie(name st
 	return client
 }

-func (b OpenshiftControllerClientBuilder) OpenshiftTemplateClient(name string) (templateclient.Interface, error) {
+func (b OpenshiftControllerClientBuilder) OpenshiftInternalTemplateClient(name string) (templateclient.Interface, error) {


Golint comments: exported method OpenshiftControllerClientBuilder.OpenshiftInternalTemplateClient should have comment or be unexported. More info.

openshift-ci-robot · 2017-09-02T22:08:03Z

pkg/cmd/server/origin/controller/interfaces.go

 	clientConfig, err := b.Config(name)
 	if err != nil {
 		return nil, err
 	}
 	return templateclient.NewForConfig(clientConfig)
 }

-func (b OpenshiftControllerClientBuilder) OpenshiftTemplateClientOrDie(name string) templateclient.Interface {
-	client, err := b.OpenshiftTemplateClient(name)
+func (b OpenshiftControllerClientBuilder) OpenshiftInternalTemplateClientOrDie(name string) templateclient.Interface {


Golint comments: exported method OpenshiftControllerClientBuilder.OpenshiftInternalTemplateClientOrDie should have comment or be unexported. More info.

openshift-ci-robot · 2017-09-02T22:08:03Z

pkg/cmd/server/origin/controller/interfaces.go

+	return client
+}
+
+func (b OpenshiftControllerClientBuilder) OpenshiftInternalImageClient(name string) (imageclientinternal.Interface, error) {


Golint comments: exported method OpenshiftControllerClientBuilder.OpenshiftInternalImageClient should have comment or be unexported. More info.

openshift-ci-robot · 2017-09-02T22:08:03Z

pkg/cmd/server/origin/controller/interfaces.go

+	return imageclientinternal.NewForConfig(clientConfig)
+}
+
+func (b OpenshiftControllerClientBuilder) OpenshiftInternalImageClientOrDie(name string) imageclientinternal.Interface {


Golint comments: exported method OpenshiftControllerClientBuilder.OpenshiftInternalImageClientOrDie should have comment or be unexported. More info.

openshift-ci-robot · 2017-09-02T22:08:03Z

pkg/cmd/server/origin/controller/interfaces.go

 	if err != nil {
 		glog.Fatal(err)
 	}
 	return client
 }

-func (b OpenshiftControllerClientBuilder) OpenshiftImageClient(name string) (imageclient.Interface, error) {
+func (b OpenshiftControllerClientBuilder) OpenshiftInternalAppsClient(name string) (appsclientinternal.Interface, error) {


Golint comments: exported method OpenshiftControllerClientBuilder.OpenshiftInternalAppsClient should have comment or be unexported. More info.

openshift-ci-robot · 2017-09-02T22:08:03Z

pkg/cmd/server/origin/controller/interfaces.go

 }

-func (b OpenshiftControllerClientBuilder) OpenshiftImageClientOrDie(name string) imageclient.Interface {
-	client, err := b.OpenshiftImageClient(name)
+func (b OpenshiftControllerClientBuilder) OpenshiftInternalAppsClientOrDie(name string) appsclientinternal.Interface {


Golint comments: exported method OpenshiftControllerClientBuilder.OpenshiftInternalAppsClientOrDie should have comment or be unexported. More info.

openshift-ci-robot · 2017-09-02T22:08:03Z

test/util/client.go

@@ -64,6 +64,14 @@ func GetClusterAdminClientConfig(adminKubeConfigFile string) (*restclient.Config
 	return conf, nil
 }

+func GetClusterAdminClientConfigOrDie(adminKubeConfigFile string) *restclient.Config {


Golint comments: exported function GetClusterAdminClientConfigOrDie should have comment or be unexported. More info.

mfojtik · 2017-09-04T08:58:06Z

all linter comments fixed

@tnozicka @deads2k @Kargakis PTAL (tests should be green).

tnozicka

nits, lgtm

tnozicka · 2017-09-04T08:59:47Z

pkg/deploy/apiserver/apiserver.go

+	if err != nil {
+		return nil, err
+	}
+	// This client is using the core api server cient config, since the apps server doesn't host images


s/cient/client

tnozicka · 2017-09-04T09:11:19Z

test/integration/imagestream_test.go

@@ -406,7 +407,7 @@ func TestImageStreamTagLifecycleHook(t *testing.T) {
 	}

 	// can tag to a stream that exists
-	exec := stratsupport.NewHookExecutor(nil, clusterAdminClient, clusterAdminKubeClientset.Core(), os.Stdout, kapi.Codecs.UniversalDecoder())
+	exec := stratsupport.NewHookExecutor(nil, imageclient.NewForConfigOrDie(testutil.GetClusterAdminClientConfigOrDie(clusterAdminKubeConfig)), clusterAdminKubeClientset.Core(), os.Stdout, kapi.Codecs.UniversalDecoder())


@mfojtik can you extract imageclient.NewForConfigOrDie(testutil.GetClusterAdminClientConfigOrDie(clusterAdminKubeConfig)) into a variable and not create it 3 times?

0xmichalis · 2017-09-04T09:29:19Z

/shrug

0xmichalis · 2017-09-04T09:29:38Z

/unshrug

openshift-ci-robot · 2017-09-04T09:29:38Z

@Kargakis: ¯\_(ツ)_/¯

In response to this:

/unshrug

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.

mfojtik · 2017-09-04T10:19:44Z

/retest

mfojtik · 2017-09-04T11:23:38Z

/retest

mfojtik · 2017-09-04T19:08:43Z

/retest

deads2k · 2017-09-05T14:05:04Z

...rnetes/staging/src/k8s.io/kube-gen/cmd/client-gen/generators/fake/generator_fake_for_type.go

@@ -420,8 +420,8 @@ var createSubresourceTemplate = `
 // Create takes the representation of a $.inputType|private$ and creates it.  Returns the server's representation of the $.resultType|private$, and an error, if there is any.
 func (c *Fake$.type|publicPlural$) Create($.type|private$Name string, $.inputType|private$ *$.inputType|raw$) (result *$.resultType|raw$, err error) {
 	obj, err := c.Fake.
-		$if .namespaced$Invokes($.NewCreateSubresourceAction|raw$($.type|allLowercasePlural$Resource, $.type|private$Name, "$.subresourcePath$", c.ns, $.inputType|private$), &$.inputType|raw${})


What's the deal here? This is fixed in upstream already? Why not pick that one back?

it is fixed in upstream PR, unfortunately after the downstream merged....

@deads2k so this will be a drop on rebase because the original upstream change include this fix.

@deads2k so this will be a drop on rebase because the original upstream change include this fix.

ok

soltysh

One question, otherwise lgtm

soltysh · 2017-09-05T11:12:01Z

...rnetes/staging/src/k8s.io/kube-gen/cmd/client-gen/generators/fake/generator_fake_for_type.go

-		$if .namespaced$Invokes($.NewCreateSubresourceAction|raw$($.type|allLowercasePlural$Resource, $.type|private$Name, "$.subresourcePath$", c.ns, $.inputType|private$), &$.inputType|raw${})
-		$else$Invokes($.NewRootCreateAction|raw$($.inputType|allLowercasePlural$Resource, $.inputType|private$), &$.inputType|raw${})$end$
+		$if .namespaced$Invokes($.NewCreateSubresourceAction|raw$($.type|allLowercasePlural$Resource, $.type|private$Name, "$.subresourcePath$", c.ns, $.inputType|private$), &$.resultType|raw${})
+		$else$Invokes($.NewRootCreateAction|raw$($.inputType|allLowercasePlural$Resource, $.inputType|private$), &$.resultType|raw${})$end$


Why only this part and not entire kubernetes/kubernetes#51638?

not sure what are you asking. that part is already merged :)

deads2k · 2017-09-05T15:05:47Z

/lgtm

openshift-merge-robot · 2017-09-05T15:06:00Z

[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.

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

deads2k · 2017-09-05T15:09:03Z

/retest

openshift-merge-robot · 2017-09-05T20:33:27Z

Automatic merge from submit-queue (batch tested with PRs 16142, 16100, 16109, 16113, 16117)

@deads2k

Automatic merge from submit-queue apps: replace legacy client with generated in pkg/deploy/cmd Ignore first 3 commits, they are part of #16100 @deads2k last bits in apps group

openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 1, 2017

openshift-merge-robot assigned smarterclayton and liggitt Sep 1, 2017

openshift-merge-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-api-review labels Sep 1, 2017

mfojtik commented Sep 1, 2017

View reviewed changes

mfojtik changed the title ~~apps: switch to use generated internal clientset~~ WIP: apps: switch to use generated internal clientset Sep 1, 2017

deads2k reviewed Sep 1, 2017

View reviewed changes

mfojtik force-pushed the deploy-clientset branch 4 times, most recently from 60ac32f to 88d4bd7 Compare September 1, 2017 22:22

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

openshift-merge-robot removed the needs-api-review label Sep 1, 2017

mfojtik changed the title ~~WIP: apps: switch to use generated internal clientset~~ apps: switch to use generated internal clientset Sep 1, 2017

openshift-ci-robot reviewed Sep 2, 2017

View reviewed changes

mfojtik added 2 commits September 4, 2017 10:50

UPSTREAM: <drop>: Fix result type in fake clientset generator

c1e422f

regenerate fake deploymentconfig

30b40b8

mfojtik force-pushed the deploy-clientset branch from 88d4bd7 to ef6504d Compare September 4, 2017 08:57

tnozicka reviewed Sep 4, 2017

View reviewed changes

openshift-ci-robot added the ¯\_(ツ)_/¯ ¯\\\_(ツ)_/¯ label Sep 4, 2017

openshift-ci-robot removed the ¯\_(ツ)_/¯ ¯\\\_(ツ)_/¯ label Sep 4, 2017

apps: use generate clientset in controllers and api server

f11ddbf

mfojtik force-pushed the deploy-clientset branch from ef6504d to f11ddbf Compare September 4, 2017 11:25

deads2k reviewed Sep 5, 2017

View reviewed changes

soltysh approved these changes Sep 5, 2017

View reviewed changes

openshift-ci-robot assigned deads2k Sep 5, 2017

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

mfojtik mentioned this pull request Sep 5, 2017

apps: replace legacy client with generated in pkg/deploy/cmd #16149

Merged

openshift-merge-robot merged commit f1d3d0d into openshift:master Sep 5, 2017

mfojtik deleted the deploy-clientset branch September 5, 2018 21:07

apps: switch to use generated internal clientset #16100

apps: switch to use generated internal clientset #16100

Conversation

mfojtik commented Sep 1, 2017 • edited Loading

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

deads2k commented Sep 1, 2017

mfojtik commented Sep 1, 2017

0xmichalis commented Sep 2, 2017

openshift-ci-robot left a 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 Sep 4, 2017

tnozicka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xmichalis commented Sep 4, 2017

0xmichalis commented Sep 4, 2017

openshift-ci-robot commented Sep 4, 2017

mfojtik commented Sep 4, 2017

mfojtik commented Sep 4, 2017

mfojtik commented Sep 4, 2017

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

soltysh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Sep 5, 2017

openshift-merge-robot commented Sep 5, 2017

deads2k commented Sep 5, 2017

openshift-merge-robot commented Sep 5, 2017

mfojtik commented Sep 1, 2017 •

edited

Loading