Use image outputs in examples #217

bobcatfish · 2018-11-02T23:50:10Z

While talking with @jonjohnsonjr about #216 I wanted to show him an
example of a Pipeline wherein there are Tasks that output Images
Resources, which are then used as inputs for subsequent Tasks. Our
examples Pipelines were meant to do that, but unfortunately when I
looked at them, I realized these were not actually hooked up properly.
The Tasks were not actually using the Images that were built by previous
Tasks, so I have updated them to do that.

I also discovered that the templating was incorrect (#108 - adding tests
for these examples - and #212 - making sure templated variables are
actually used - can't come quickly enough!) so I've updated that in the
examples as well.

knative-prow-robot · 2018-11-02T23:50:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish

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~~ [bobcatfish]

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

bobcatfish · 2018-11-03T00:22:38Z

W1103 00:02:54.849] error: error validating "STDIN": error validating data: [ValidationError(CustomResourceDefinition.status): missing required field "conditions" in io.k8s.apiextensions-apiserver.pkg.apis.apiextensions.v1beta1.CustomResourceDefinitionStatus, ValidationError(CustomResourceDefinition.status): missing required field "storedVersions" in io.k8s.apiextensions-apiserver.pkg.apis.apiextensions.v1beta1.CustomResourceDefinitionStatus]; if you choose to ignore these errors, turn validation off with --validate=false
W1103 00:02:54.850] 2018/11/02 23:57:35 error executing "kubectl apply": exit status 1

wut

bobcatfish · 2018-11-03T18:21:22Z

examples/pipelines/guestbook.yaml

        - name: deploy-bundle-test               # 2. Deploy GuestBook and Redis to test cluster
          taskRef:
              name: deploy-with-kubectl
          inputSourceBindings:
-            - name: workspace
+            - name: imageToDeploy1


whoops, these should be redisImage and guestbookImage

nader-ziada · 2018-11-05T13:48:54Z

/test pull-knative-build-pipeline-integration-tests

nader-ziada · 2018-11-05T14:50:25Z

/test pull-knative-build-pipeline-integration-tests

bobcatfish · 2018-11-05T16:26:48Z

/test pull-knative-build-pipeline-integration-tests

nader-ziada · 2018-11-05T16:39:54Z

@bobcatfish everything looks fine to me in the yaml examples. but the test is failing for what seems to be an infrastructure issue

bobcatfish · 2018-11-05T17:30:40Z

I think there are 2 problems, 1 is an infrastructure problem, the other is that the k8s clusters were upgraded to 1.11 and I think we're running into something similar to kubevirt/kubevirt#1279

adrcunha · 2018-11-05T17:33:41Z

Since 5AM all CI flows are failing when creating a k8s cluster. I'll investigate.

bobcatfish · 2018-11-05T20:00:47Z

Thanks @adrcunha - looks like https://status.cloud.google.com//incident/compute/18012?_ga=2.196962540.-1744253383.1524667138 is over now, so we'll maybe be back to the 1.11 issue which I can look into:

/test pull-knative-build-pipeline-integration-tests

bobcatfish · 2018-11-05T20:02:06Z

examples/deploy_tasks.yaml

+          - "'s/image: k8s.gcr.io\/redis:e2e/image: ${inputs.resources.redisImage.url}@{inputs.resources.redisImage.digest}/' ${inputs.params.pathToFiles}"
+        - name: replaceGuestbookImage
+          image: busybox
+          command: ['sed']


feedback from @jonjohnsonjr : I'm going to at least TRY using kustomize before I resign myself to sed here :D

bobcatfish · 2018-11-05T23:47:33Z

Should be fixed by #226 🤞

/test pull-knative-build-pipeline-integration-tests

nader-ziada · 2018-11-06T14:13:53Z

examples/test_tasks.yaml

@@ -34,9 +34,8 @@ spec:
        resources:
           - name: workspace
             type: git
+           - name: testImage


the test is failing because the testImage here needs a type of image

bobcatfish · 2018-11-07T01:17:53Z

PTAL @shashwathi @pivotal-nader-ziada

nader-ziada

just a few minor comments

nader-ziada · 2018-11-07T14:05:07Z

examples/deploy_tasks.yaml

+          - '--namespace'
+          - '${inputs.clusters.targetCluster.namespace}'
+          - 'apply'
+          - '-c'


should this -c be -f?

nader-ziada · 2018-11-07T14:11:25Z

examples/pipelines/kritis-resources.yaml

-  - name: url
-    value: https://github.com/grafeas/kritis-test  
---    
+---


There are use cases when having tests in a separate repo is valid, for example having acceptance test that make sure products from different repos integrate and deploy properly. Anyways doesn't have to be part of our example, but it was good way to show using multiple git repos in the same task

yeah i think that makes perfect sense - I'd like to go ahead with this change since https://github.com/grafeas/kritis-test doesn't exist, and it's kinda nice to have the examples use real world repos, but if you feel strongly I can leave it as is (with a comment maybe!)

I’m okay with your change :)

nader-ziada · 2018-11-07T14:14:11Z

examples/test_tasks.yaml

            - name: cloudconfig
              hostPath:
-                path: ${workspace}/config/gcloud
+                path: /workspace/config/gcloud


the path here refers to /workspace while the volume mount int the step refers to /root

hm I think that's okay, the docs for hostPath (here) say:

HostPath represents a pre-existing file or directory on the host machine that is directly exposed to the container.

So I think what this is doing is creating a volume from a dir that exists already on the host (which was mounted into /workspace via the resource), then in volume mounts it's mounting this dir into a specific location in /root

nader-ziada · 2018-11-07T15:51:09Z

examples/build_task.yaml

@@ -23,5 +23,5 @@ spec:
      command:
      - /kaniko/executor
      args:
-      - --dockerfile=${pathToDockerFile}
-      - --destination=$builtImage
+      - --dockerfile=${inputs.resources.pathToDockerFile}


this should be inputs.params

good catch!

@jonjohnsonjr

While talking with @jonjohnsonjr about tektoncd#216 I wanted to show him an example of a Pipeline wherein there are Tasks that output Images Resources, which are then used as inputs for subsequent Tasks. Our examples Pipelines were meant to do that, but unfortunately when I looked at them, I realized these were not actually hooked up properly. The Tasks were not actually using the Images that were built by previous Tasks, so I have updated them to do that. I also discovered that the templating was incorrect (#108 - adding tests for these examples - and tektoncd#212 - making sure templated variables are actually used - can't come quickly enough!) so I've updated that in the examples as well.

nader-ziada · 2018-11-07T18:06:35Z

/lgtm

knative-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 2, 2018

knative-prow-robot requested review from dlorenc and imjasonh November 2, 2018 23:50

bobcatfish commented Nov 3, 2018

View reviewed changes

bobcatfish commented Nov 5, 2018

View reviewed changes

bobcatfish mentioned this pull request Nov 5, 2018

Can't deploy after move to k8s 1.11 #225

Closed

nader-ziada reviewed Nov 6, 2018

View reviewed changes

bobcatfish mentioned this pull request Nov 7, 2018

Initial design ideas for Source #39

Merged

bobcatfish force-pushed the outputs_in_examples branch from cdf57d8 to 30e13da Compare November 7, 2018 01:17

bobcatfish force-pushed the outputs_in_examples branch from 30e13da to 09d4d46 Compare November 7, 2018 02:23

nader-ziada reviewed Nov 7, 2018

View reviewed changes

bobcatfish force-pushed the outputs_in_examples branch from 09d4d46 to 3e4657c Compare November 7, 2018 17:16

knative-prow-robot assigned nader-ziada Nov 7, 2018

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

knative-prow-robot merged commit 7af42f1 into tektoncd:master Nov 7, 2018

mchmarny unassigned nader-ziada Mar 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use image outputs in examples #217

Use image outputs in examples #217

bobcatfish commented Nov 2, 2018

knative-prow-robot commented Nov 2, 2018

bobcatfish commented Nov 3, 2018

bobcatfish Nov 3, 2018

nader-ziada commented Nov 5, 2018

nader-ziada commented Nov 5, 2018

bobcatfish commented Nov 5, 2018

nader-ziada commented Nov 5, 2018

bobcatfish commented Nov 5, 2018

adrcunha commented Nov 5, 2018

bobcatfish commented Nov 5, 2018

bobcatfish Nov 5, 2018

bobcatfish commented Nov 5, 2018

nader-ziada Nov 6, 2018

bobcatfish commented Nov 7, 2018

nader-ziada left a comment

nader-ziada Nov 7, 2018

bobcatfish Nov 7, 2018

nader-ziada Nov 7, 2018

bobcatfish Nov 7, 2018

nader-ziada Nov 7, 2018

nader-ziada Nov 7, 2018

bobcatfish Nov 7, 2018

nader-ziada Nov 7, 2018

bobcatfish Nov 7, 2018

nader-ziada commented Nov 7, 2018

Use image outputs in examples #217

Use image outputs in examples #217

Conversation

bobcatfish commented Nov 2, 2018

knative-prow-robot commented Nov 2, 2018

bobcatfish commented Nov 3, 2018

Choose a reason for hiding this comment

nader-ziada commented Nov 5, 2018

nader-ziada commented Nov 5, 2018

bobcatfish commented Nov 5, 2018

nader-ziada commented Nov 5, 2018

bobcatfish commented Nov 5, 2018

adrcunha commented Nov 5, 2018

bobcatfish commented Nov 5, 2018

Choose a reason for hiding this comment

bobcatfish commented Nov 5, 2018

Choose a reason for hiding this comment

bobcatfish commented Nov 7, 2018

nader-ziada 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

nader-ziada commented Nov 7, 2018