Add integration test to run a helm deploy pipeline #160

nader-ziada · 2018-10-17T18:23:58Z

Related to: #63

Proposed changes

Create a pipeline of two tasks,
First task build and push an image
Second task helm deploys that image
Verify the service created is reachable

/cc @bobcatfish

knative-prow-robot · 2018-10-17T18:24:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pivotal-nader-ziada

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [pivotal-nader-ziada]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

nader-ziada · 2018-10-17T18:36:50Z

/cc @shashwathi

bobcatfish

I LOVE that all the helm setup is being done via the Pipeline itself! So cool!!

Just a few minor comments, only big thing is the cleanup request :D

/meow space

😍

bobcatfish · 2018-10-17T18:44:23Z

test/crd_checks.go

@@ -30,7 +30,7 @@ import (

 const (
 	interval = 1 * time.Second
-	timeout  = 2 * time.Minute
+	timeout  = 4 * time.Minute


at this point, what do you think about adding a timeout argument to the polling functions, so that each test can pass its own timeout? e.g. the helloworld task probably needs < 1 minute, kaniko needs 2 minutes, this one needs 4 minutes. it will help to keep track of why the timeout is what it is, and also will ensure tests don't wait for longer than they need to

test/helm_task_test.go

bobcatfish · 2018-10-17T18:47:01Z

test/helm_task_test.go

+					Name:  "kaniko",
+					Image: "gcr.io/kaniko-project/executor",
+					Args: []string{"--dockerfile=/workspace/Dockerfile",
+						fmt.Sprintf("--destination=%s", imageName),


if you wanted, we have templating working now, so you could use Task params for the image name and path

bobcatfish · 2018-10-17T18:49:45Z

test/helm_task_test.go

+							"--set",
+							"image.repository=${inputs.params.image}",
+						},
+					}},


i think something is off with the indentation b/w these

yes, fixed it

bobcatfish · 2018-10-17T18:50:37Z

test/helm_task_test.go

+					Name:  "helm-init",
+					Image: "alpine/helm",
+					Args:  []string{"init"},
+				},


i wonder if the teardown step in the test should invoke a Task or something to remove helm, so that ppl who run this test aren't left with helm running in their clusters (or will the namespace take care of that?)

no, the namespace will not take care of that, but I was worried if if things fail running another task might not work for whatever reason and I wasn't sure how else to remove it

doesn't the cluster get deleted after the test run in Prow?

I was worried if if things fail running another task might not work for whatever reason and I wasn't sure how else to remove it

That's true, so a couple options:

Try to do it anyway but don't actually fail anything if the teardown fails (just log it)

Try to do teardown without using Pipeline/Task, e.g. call out to kubectl

doesn't the cluster get deleted after the test run in Prow?

Yeah I think so! So we're fine for Prow, I'm more concerned about ppl running this on their own clusters.

will get the test green first then add the cleanup

bobcatfish · 2018-10-17T18:51:49Z

test/helm_task_test.go

+	}
+}
+
+func getelmDeployPipeline(namespace string) *v1alpha1.Pipeline {


knative-prow-robot · 2018-10-17T18:52:44Z

@bobcatfish: Bad category. Please see http://thecatapi.com/api/categories/list

In response to this:

I LOVE that all the helm setup is being done via the Pipeline itself! So cool!!

Just a few minor comments, only big thing is the cleanup request :D

/meow space

😍

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.

tejal29 · 2018-10-17T18:47:31Z

test/crd_checks.go

@@ -30,7 +30,7 @@ import (

 const (
 	interval = 1 * time.Second
-	timeout  = 2 * time.Minute
+	timeout  = 4 * time.Minute


Is there a reason why we had to increase the timeout?

test/crd_checks.go

tejal29 · 2018-10-17T18:50:17Z

test/helm_task_test.go

+	}
+
+	logger.Infof("Creating Pipeline %s", helmDeployPipelineName)
+	if _, err := c.PipelineClient.Create(getelmDeployPipeline(namespace)); err != nil {


spelling mistake. getelmDeployPipeline

tejal29 · 2018-10-17T18:51:43Z

test/helm_task_test.go

+		t.Errorf("Error getting service at %s %s", helmDeployServiceName, err)
+	}
+	var serviceIp string
+	ingress := k8sService.Status.LoadBalancer.Ingress


this might result in null pointer exception if there was an error above in L97.

will add a check

test/helm_task_test.go

tejal29 · 2018-10-17T19:00:40Z

test/helm_task_test.go

+	}
+}
+
+func getelmDeployPipeline(namespace string) *v1alpha1.Pipeline {


fix spell error.

tejal29 · 2018-10-17T19:00:58Z

test/helm_task_test.go

+	}
+}
+
+func getelmDeployPipelineRun(namespace string) *v1alpha1.PipelineRun {


fix spell error

bobcatfish · 2018-10-17T19:01:39Z

wut

/meow space

bobcatfish · 2018-10-17T19:02:00Z

/meow space

knative-prow-robot · 2018-10-17T19:02:00Z

@bobcatfish:

In response to this:

/meow space

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.

nader-ziada · 2018-10-18T20:27:51Z

Added cleanup task after the test is runs to remove tiller from cluster

bobcatfish

Looks like this is almost done!!

pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinestate.go

bobcatfish · 2018-10-18T20:44:06Z

test/helm_task_test.go

+	}
+
+	// cleanup task to remove helm from cluster, will not fail the test if it fails, just log
+	removeHelmFromCluster(c, t, namespace, logger)


i think we should make sure this runs if the test is interrupted or fails, e.g. like we do with the teardown (and/or move it to the teardown):

knativetest.CleanupOnInterrupt(func() { tearDown(logger, c.KubeClient, namespace) }, logger) defer tearDown(logger, c.KubeClient, namespace)

- tektoncd#63 - create a pipeline of two tasks, - first task build and push an image - second task helm deploys that image - verify the service created is reachable

- Before a pipeline run creates a taskrun for the next task, it needs to check the passed constraints to respect dependecnies between tasks - add implementation of canTaskRun and unit tests - add a task to delete tiller at the end of the helm task test

- call helm cleanup if test is interrupted or fails - cleanup service account and cluster role binding

bobcatfish · 2018-10-19T16:05:31Z

Just a quick note that we should leave #63 open after this b/c we still need to do some work to respect the cluster parameters (e.g. mount and create a kubeconfig that helm can use?). At the moment, we can only deploy to the cluster that the Pipeline CRD is already running in.

(Apologies if I added this comment already, I thought I did but I can't find it 😅 )

nader-ziada · 2018-10-19T16:16:49Z

@bobcatfish should the different cluster be a different issue since this is not yet implemented in the controllers?

nader-ziada · 2018-10-19T17:14:00Z

/hold

will follow up with another pr to update helm test to use this instead of external repo, but need it to be in git source

nader-ziada · 2018-10-19T19:52:59Z

/retest

nader-ziada · 2018-10-19T20:24:49Z

@tejal29 can you take another look, pr is ready for review now. Thanks

The only thing remaining is to not use the external repo, but will have to do another pr to change that because we need the folder in test to exist first in the soure repo.

nader-ziada · 2018-10-19T20:25:07Z

/hold cancel

tejal29

Perfect!
/lgtm

shashwathi · 2018-10-19T22:04:44Z

This PR also addresses : #148

bobcatfish · 2018-10-22T14:05:26Z

@bobcatfish should the different cluster be a different issue since this is not yet implemented in the controllers?

sure that's fine!

knative-prow-robot requested a review from bobcatfish October 17, 2018 18:23

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 17, 2018

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 17, 2018

knative-prow-robot requested a review from shashwathi October 17, 2018 18:36

nader-ziada force-pushed the helm-task branch from a772002 to bfc3646 Compare October 17, 2018 18:47

bobcatfish reviewed Oct 17, 2018

View reviewed changes

tejal29 suggested changes Oct 17, 2018

View reviewed changes

tektoncd deleted a comment from knative-prow-robot Oct 17, 2018

nader-ziada force-pushed the helm-task branch 4 times, most recently from eb2c3b6 to 0f03523 Compare October 18, 2018 14:16

knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 18, 2018

nader-ziada force-pushed the helm-task branch 3 times, most recently from d45879f to 323f693 Compare October 18, 2018 20:01

nader-ziada force-pushed the helm-task branch from 323f693 to 8da149e Compare October 18, 2018 20:33

bobcatfish reviewed Oct 18, 2018

View reviewed changes

nader-ziada added 3 commits October 19, 2018 09:16

Add integration test to run a helm deploy pipeline

a2e8ec5

- tektoncd#63 - create a pipeline of two tasks, - first task build and push an image - second task helm deploys that image - verify the service created is reachable

changes to helm task test to improve reliability and fix typos

c6807ab

nader-ziada force-pushed the helm-task branch from 8da149e to 725a946 Compare October 19, 2018 13:17

better cleanup of helm artifacts from cluster in test

1427334

- call helm cleanup if test is interrupted or fails - cleanup service account and cluster role binding

nader-ziada force-pushed the helm-task branch from 725a946 to 1427334 Compare October 19, 2018 15:14

dumping build logs to debug failure on Prow

a5478f8

nader-ziada force-pushed the helm-task branch from 5c1ccac to a5478f8 Compare October 19, 2018 16:12

add --wait flag on helm init to block until tiller is ready

5f218cb

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 19, 2018

cleanup of logging in helm task test

4adfd60

nader-ziada force-pushed the helm-task branch from b85dbad to 4adfd60 Compare October 19, 2018 19:20

Add gohelloworld sample in test folder

3f87858

will follow up with another pr to update helm test to use this instead of external repo, but need it to be in git source

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 19, 2018

tejal29 approved these changes Oct 19, 2018

View reviewed changes

knative-prow-robot assigned tejal29 Oct 19, 2018

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 19, 2018

shashwathi mentioned this pull request Oct 19, 2018

Link outputSourceBindings from one PipelineTask to inputSourceBindings of next task #148

Closed

knative-prow-robot merged commit 6e96911 into tektoncd:master Oct 20, 2018

bobcatfish mentioned this pull request Oct 22, 2018

Replace Pipeline linear execution with graph based execution #168

Closed

nader-ziada mentioned this pull request Oct 22, 2018

Implement working Task that deploys to k8s using helm #63

Closed

nader-ziada deleted the helm-task branch November 29, 2018 20:03

mchmarny unassigned tejal29 Mar 7, 2019

Add integration test to run a helm deploy pipeline #160

Add integration test to run a helm deploy pipeline #160

Conversation

nader-ziada commented Oct 17, 2018 • edited Loading

Proposed changes

knative-prow-robot commented Oct 17, 2018

nader-ziada commented Oct 17, 2018

bobcatfish 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knative-prow-robot commented Oct 17, 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

bobcatfish commented Oct 17, 2018

bobcatfish commented Oct 17, 2018

knative-prow-robot commented Oct 17, 2018

nader-ziada commented Oct 18, 2018

bobcatfish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobcatfish commented Oct 19, 2018

nader-ziada commented Oct 19, 2018

nader-ziada commented Oct 19, 2018

nader-ziada commented Oct 19, 2018

nader-ziada commented Oct 19, 2018

nader-ziada commented Oct 19, 2018

tejal29 left a comment

Choose a reason for hiding this comment

shashwathi commented Oct 19, 2018

bobcatfish commented Oct 22, 2018

nader-ziada commented Oct 17, 2018 •

edited

Loading