added support for OpenShift down #280

procrypt · 2016-11-14T09:44:09Z

Fix #208
cc @kadel @surajssd @sebgoa @ngtuna

surajssd · 2016-11-17T05:27:53Z

pkg/transformer/openshift/openshift.go

+			}
+		case *deployapi.DeploymentConfig:
+			// delete deploymentConfig
+			err = oclient.DeploymentConfigs(namespace).Delete(t.Name)


Using reaper function Stop would be better, than just deleting it right away.

I tried something like this

diff --git a/pkg/transformer/openshift/openshift.go b/pkg/transformer/openshift/openshift.go index 8a81fef..992c364 100644 --- a/pkg/transformer/openshift/openshift.go +++ b/pkg/transformer/openshift/openshift.go @@ -35,11 +35,13 @@ import ( oclient "github.com/openshift/origin/pkg/client" ocliconfig "github.com/openshift/origin/pkg/cmd/cli/config" + deploymentconfigreaper "github.com/openshift/origin/pkg/deploy/cmd" + + "time" deployapi "github.com/openshift/origin/pkg/deploy/api" imageapi "github.com/openshift/origin/pkg/image/api" "k8s.io/kubernetes/pkg/kubectl" - "time" ) type OpenShift struct { @@ -252,7 +254,6 @@ func (o *OpenShift) Undeploy(komposeObject kobject.KomposeObject, opt kobject.Co } oclient := oclient.NewOrDie(oclientConfig) - // initialize Kubernetes client kfactory := kcmdutil.NewFactory(nil) kclientConfig, err := kfactory.ClientConfig() @@ -272,14 +273,16 @@ func (o *OpenShift) Undeploy(komposeObject kobject.KomposeObject, opt kobject.Co case *imageapi.ImageStream: //delete imageStream err = oclient.ImageStreams(namespace).Delete(t.Name) - if err != nil { + if err != nil { return err } else { logrus.Infof("Successfully deleted ImageStream: %s", t.Name) } case *deployapi.DeploymentConfig: // delete deploymentConfig - err = oclient.DeploymentConfigs(namespace).Delete(t.Name) + dcreaper := deploymentconfigreaper.NewDeploymentConfigReaper(oclient, kclient) + err := dcreaper.Stop(namespace, t.Name, TIMEOUT*time.Second, nil) + //err = oclient.DeploymentConfigs(namespace).Delete(t.Name) if err != nil { return err } else {

@surajssd I'll Update the PR.

procrypt · 2016-11-21T05:36:45Z

@surajssd I have updated the PR.

surajssd · 2016-11-21T06:16:37Z

We need tests for this PR to go in.

kadel · 2016-11-22T14:38:23Z

pkg/transformer/openshift/openshift.go

+	//Convert komposeObject
+	objects := o.Transform(komposeObject, opt)
+
+	// initialize OpenShift Client


Same code for initializing openshift client is used in Deploy function. It might be better to move this to separate function (for example (o *OpenShift) getOpenShiftClient (*Client, error) ) and make Deploy and Undeploy call it.

Same thing for Kubernetes client.

@kadel I'll update the PR.

procrypt · 2016-11-25T08:32:12Z

@surajssd Working on tests.

procrypt · 2016-11-28T10:51:17Z

@kadel Updated the PR

procrypt · 2016-11-28T10:51:50Z

@kadel Updated the PR.
Please review :)

procrypt · 2016-12-02T07:51:56Z

@kadel I think it will be a heavy job, to bring up the OpenShift cluster for unit testing of the code. What we can do is, open a separate issue for the testing part and not block this PR for unit tests.

Thoughts ?

kadel

OK, I guess this will be great candidate for future functional tests. We can figure add that in separate PR once we have idea how we will do that.

Just on last thing, it would be great to add few comments to code. (see my comments)

And it needs rebase ;-)

Otherwise LGTM

kadel · 2016-12-02T08:46:26Z

pkg/transformer/kubernetes/kubernetes.go

@@ -373,6 +373,23 @@ func (k *Kubernetes) UpdateController(obj runtime.Object, updateTemplate func(*a
 	}
 }

+func (o *Kubernetes) GetKubernetesClient() (*client.Client, string, error) {


Can you please add comment to this function explaining what it does, and what are the return values

kadel · 2016-12-02T08:48:35Z

pkg/transformer/openshift/openshift.go

-	fmt.Println("We are going to create OpenShift DeploymentConfigs, Services and PersistentVolumeClaims for your Dockerized application. \n" +
-		"If you need different kind of resources, use the 'kompose convert' and 'oc create -f' commands instead. \n")
-
+func (o *OpenShift) getOpenShiftClient() (*oclient.Client, error) {


Can you please add comment to this function explaining what it does, and what are the return values?

procrypt · 2016-12-02T10:07:16Z

@kadel added comments.

kadel · 2016-12-05T09:59:16Z

@procrypt gofmt 😉

kadel

LGTM.
But it would be great to have second pair of eyes to check it. @surajssd

surajssd · 2016-12-12T12:05:27Z

pkg/transformer/openshift/openshift.go

+	if err != nil {
+		return err
+	}
+	kclient, namespace, _ := o.Kubernetes.GetKubernetesClient()


why are you ignoring error here?

Also o.GetKubernetesClient() should work, it will call the original function!

@surajssd I didn't notice that, thanks for the trace 👍
and yeah o.GetKubernetesClient(), it works, I have updated the PR.

surajssd · 2016-12-12T12:06:16Z

pkg/transformer/openshift/openshift.go

+		return err
+	}
+	kclient, namespace, _ := o.Kubernetes.GetKubernetesClient()
+	if err != nil {


since error is ignored above the value err has is from line oclient, err := o.getOpenShiftClient()

surajssd · 2016-12-12T12:08:00Z

pkg/transformer/openshift/openshift.go

-
-	// get namespace from config
-	namespace, _, err := kfactory.DefaultNamespace()
+	kclient, namespace, _ := o.Kubernetes.GetKubernetesClient()


see comments below on similar issue!

@surajssd PR updated

coveralls · 2016-12-12T12:41:31Z

Coverage decreased (-0.03%) to 34.975% when pulling 27dc5dc on procrypt:down into 04a3131 on kubernetes-incubator:master.

kadel · 2016-12-12T17:26:57Z

@surajssd Good catch. When you resolve those issues with @procrypt. Feel free to merge it.

coveralls · 2016-12-15T05:44:52Z

Coverage decreased (-1.3%) to 33.656% when pulling 6ad54a3 on procrypt:down into 65e19e3 on kubernetes-incubator:master.

coveralls · 2016-12-15T05:49:53Z

Coverage decreased (-1.3%) to 33.656% when pulling 6ad54a3 on procrypt:down into 65e19e3 on kubernetes-incubator:master.

coveralls · 2016-12-15T06:02:49Z

Coverage decreased (-1.3%) to 33.656% when pulling 6ad54a3 on procrypt:down into 65e19e3 on kubernetes-incubator:master.

surajssd · 2016-12-15T06:23:33Z

@procrypt thanks merging!

surajssd reviewed Nov 17, 2016

View reviewed changes

procrypt force-pushed the down branch from a4ae1f4 to 393ff5c Compare November 21, 2016 05:29

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 21, 2016

procrypt force-pushed the down branch from 393ff5c to 95ce96b Compare November 21, 2016 05:31

kadel requested changes Nov 22, 2016

View reviewed changes

procrypt force-pushed the down branch from 95ce96b to 0d2cd2f Compare November 28, 2016 10:49

procrypt closed this Nov 28, 2016

procrypt reopened this Nov 28, 2016

surajssd added the rebase needed label Nov 29, 2016

kadel requested changes Dec 2, 2016

View reviewed changes

procrypt force-pushed the down branch from 0d2cd2f to b444cfe Compare December 2, 2016 10:04

procrypt mentioned this pull request Dec 5, 2016

Functional tests for OpenShift down #323

Closed

procrypt force-pushed the down branch 2 times, most recently from b444cfe to 73da23b Compare December 5, 2016 07:11

procrypt force-pushed the down branch from 73da23b to 3733e46 Compare December 5, 2016 11:10

kadel removed the rebase needed label Dec 5, 2016

procrypt closed this Dec 6, 2016

procrypt deleted the down branch December 6, 2016 13:56

procrypt restored the down branch December 6, 2016 13:58

procrypt reopened this Dec 6, 2016

kadel approved these changes Dec 12, 2016

View reviewed changes

procrypt force-pushed the down branch from 3733e46 to 27dc5dc Compare December 12, 2016 10:55

surajssd reviewed Dec 12, 2016

View reviewed changes

added support for OpenShift down

6ad54a3

procrypt force-pushed the down branch 2 times, most recently from b4e7366 to 6ad54a3 Compare December 15, 2016 05:43

procrypt force-pushed the down branch from 874f267 to 6ad54a3 Compare December 15, 2016 05:57

surajssd merged commit 8676ae8 into kubernetes:master Dec 15, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added support for OpenShift down #280

added support for OpenShift down #280

procrypt commented Nov 14, 2016

surajssd Nov 17, 2016

procrypt Nov 17, 2016

procrypt commented Nov 21, 2016

surajssd commented Nov 21, 2016

kadel Nov 22, 2016

procrypt Nov 25, 2016

procrypt commented Nov 25, 2016

procrypt commented Nov 28, 2016

procrypt commented Nov 28, 2016

procrypt commented Dec 2, 2016 •

edited

Loading

kadel left a comment •

edited

Loading

kadel Dec 2, 2016

kadel Dec 2, 2016

procrypt commented Dec 2, 2016

kadel commented Dec 5, 2016

kadel left a comment

surajssd Dec 12, 2016

surajssd Dec 12, 2016

procrypt Dec 15, 2016

surajssd Dec 12, 2016

surajssd Dec 12, 2016

procrypt Dec 15, 2016

coveralls commented Dec 12, 2016

kadel commented Dec 12, 2016

coveralls commented Dec 15, 2016

coveralls commented Dec 15, 2016

coveralls commented Dec 15, 2016

surajssd commented Dec 15, 2016

added support for OpenShift down #280

added support for OpenShift down #280

Conversation

procrypt commented Nov 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

procrypt commented Nov 21, 2016

surajssd commented Nov 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

procrypt commented Nov 25, 2016

procrypt commented Nov 28, 2016

procrypt commented Nov 28, 2016

procrypt commented Dec 2, 2016 • edited Loading

kadel left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

procrypt commented Dec 2, 2016

kadel commented Dec 5, 2016

kadel 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

coveralls commented Dec 12, 2016

kadel commented Dec 12, 2016

coveralls commented Dec 15, 2016

coveralls commented Dec 15, 2016

coveralls commented Dec 15, 2016

surajssd commented Dec 15, 2016

procrypt commented Dec 2, 2016 •

edited

Loading

kadel left a comment •

edited

Loading