Migrate yaml e2e test from knative/build #493

shashwathi · 2019-02-12T17:30:40Z

Fixes #302

Add yaml tests which cover different features of knative build in knative build pipeline like mounting secret as volume, templating arguments.
I followed the pattern suggested by Jason to include relevant k8s, knative objects in same yaml separated by ---. Test file names should sufficiently explain the taskrun but if anybody feels strongly about adding more docs, please leave comment on what would be ideal place for this documentation.

I have not intentionally added all tests because either feature is not supported(like failure labels) or better examples are already present in repo like task template params.

cc @imjasonh

knative-prow-robot · 2019-02-12T17:30:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: shashwathi

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

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

nader-ziada

Thanks for migrating all these yaml files, but I found it hard to find them.

One of the benefits of these files in the build repo is to show examples of how to write your own build, or task in our case, and in their current location with no clear reference to them from any docs, most people will not see them

bobcatfish

Looks great @shashwathi ! 🎉🎉🎉

This looks like it was a lot of work, thanks for taking it on!! I think it's super cool to see proof now that we support all the same functionality of Build/BuildTemplates! I mean we're using exactly the same code as Build but still v cool to see :D

There are a few references to "build" and/or "build crd" in the examples that we might want to update
What do you think about getting rid of the run directory all together, having all the examples in the top level examples dir, one file per complete example? I think it's a bit unclear at the moment why some of the examples live at the top level and get special explanation in the README and some don't

I followed the pattern suggested by Jason to include relevant k8s, knative objects in same yaml separated by ---.

I like it! The idea of each yaml being a self contained example is nice :D

either feature is not supported(like failure labels)

I think that's reasonable! We have end to end tests, and pretty decent unit test coverage, so I think it's okay to leave out the negative examples

I have not intentionally added all tests because either feature is not supported(like failure labels) or better examples are already present in repo like task template params.

Would it be a crazy amount of work to include in the commit message which ones were skipped and why? Makes it easy to make sure none were skipped that shouldn't be, and also I can see us having to compare the examples b/w the repos again a couple more times as we migrate development completely away from knative/build

bobcatfish · 2019-02-13T17:23:13Z

examples/README.md


 It uses `kubernetes.io/dockercfg` secret type but,
 `kubernetes.io/dockerconfigjson` and
 [Knative flavored credentials](https://github.com/knative/docs/blob/master/build/auth.md#guiding-credential-selection)
 are supported too.
+
+## Results


ooo nice! maybe put a link to this section at the top?

just bumping this 😅

README is pretty small so is there any value in having link at top?

examples/run/task-volume-args.yaml

examples/run/task-env.yaml

bobcatfish · 2019-02-13T17:31:23Z

examples/run/taskrun-docker-basic.yaml

+    build.knative.dev/docker-1: https://eu.gcr.io
+    build.knative.dev/docker-2: https://asia.gcr.io
+    build.knative.dev/docker-3: https://gcr.io
+    build.knative.dev/docker-4: https://reduce-chance-of-selecting-gcr.io


i think these should be something else, maybe pipeline.knative.dev ? (not sure if this actually works tho 😅 )

It does not work. Is this something we need a separate issue or tackle it in this PR?

oh yeah separate PR then for sure i think! separate issue even

shashwathi · 2019-02-13T20:21:35Z

@bobcatfish

Would it be a crazy amount of work to include in the commit message which ones were skipped and why? Makes it easy to make sure none were skipped that shouldn't be, and also I can see us having to compare the examples b/w the repos again a couple more times as we migrate development completely away from knative/build

Done. Updated commit msg to include yamls that were not migrated

There are a few references to "build" and/or "build crd" in the examples that we might want to update

I think I have addressed it all. Let me know if I missed something

What do you think about getting rid of the run directory all together, having all the examples in the top level examples dir, one file per complete example? I think it's a bit unclear at the moment why some of the examples live at the top level and get special explanation in the README and some don'

I think there are cases like replacing image path with KO_DOCKER_REPO which needs instructions in README but I agree rest of them don't need it I guess. I tried with taskruns and pipelineruns folder separation. Is that better or worse?

Few tasks are referenced in taskruns and pipelinerun so there will be copies of same tasks repeated in multiple files. is that okay?

shashwathi · 2019-02-13T22:46:17Z

/hold cancel

shashwathi · 2019-02-13T23:48:19Z

@bobcatfish : one of build yaml tests relied on volume source templating and that feature has not implemented yet.
I have added that support in separate commit from yaml tests in this PR(also unit tests). I can do another PR too but build pipeline merges PR retaining commits so I don't think it makes much difference. If you feel otherwise I can do another PR too.

bobcatfish · 2019-02-14T19:42:21Z

pkg/reconciler/v1alpha1/taskrun/resources/apply.go

@@ -89,5 +89,14 @@ func ApplyReplacements(build *buildv1alpha1.Build, replacements map[string]strin
 			steps[i].VolumeMounts[iv].SubPath = templating.ApplyReplacements(v.SubPath, replacements)
 		}
 	}
+
+	// Apply variable expansion to the build's volumes


bobcatfish · 2019-02-14T19:46:33Z

Generally looking great but a couple doc requests:

Explaining the structure of the examples/ dir in examples/README.md (at the moment this readme kind of gives the impression that "simple pipeline" and "pipeline with output" are the only examples (I'm still not sure it makes sense to have docs in the main README on only these 2 examples, I could also see moving these descriptions to comments in the yamls themselves)
Updating the templating docs that we support volumes now too

Few tasks are referenced in taskruns and pipelinerun so there will be copies of same tasks repeated in multiple files. is that okay?

Hm I could go either way!

If we decided to not duplicate them, I think examples/README.mdshould clearly explain the structure of the dirs, i.e. how the top level files in examples/ are used across runs.

I have added that support in separate commit from yaml tests in this PR(also unit tests). I can do another PR too but build pipeline merges PR retaining commits so I don't think it makes much difference. If you feel otherwise I can do another PR too.

I'm fine with same PR but can we add some docs on this to our templating docs?

shashwathi · 2019-02-14T23:52:39Z

I'm fine with same PR but can we add some docs on this to our templating docs?

Done. Added an example. Let me know what you think

Hm I could go either way!

I have duplicated tasks and made them all self contained. README contains instruction on how to apply, what to change in yaml files and how to check results. Let me know if I missed anything.

shashwathi · 2019-02-15T15:57:16Z

@bobcatfish : #511

e2e test failure was not related to the PR. I have added an issue to fix this separately. I am going to re-trigger the tests for now

shashwathi · 2019-02-15T15:57:21Z

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

Fixes tektoncd#302 what: Add yaml tests which cover different features of knative build in knative build pipeline like mounting secret as volume, templating arguements. list build yamls tests are not added: - build with `source` key. As build added support for multiple sources build examples included both `source` and `sources` key version of build. In taskrun world it means the same thing (https://github.com/knative/build/tree/master/test/sources) - build subpath feature is not supported in taskrun (https://github.com/knative/build/tree/master/test/subpath) - build with multiple steps(https://github.com/knative/build/tree/master/test/multiple-steps). taskrun contains examples with multiple steps - build that panics (https://github.com/knative/build/tree/master/test/panic). Taskrun with expectation to fail is not supported in yaml - build with timeout (https://github.com/knative/build/tree/master/test/build-with-timeout). Taskrun with timeout test is already present in e2e go test - build with custom source. Taskrun does nto support custom source (https://github.com/knative/build/tree/master/test/custom-source) - build that fails (https://github.com/knative/build/tree/master/test/fail). Taskrun does not support failed taskruns - build with step status (https://github.com/knative/build/tree/master/test/step-status). go e2e tests already cover this feature - build yaml tests git-init image directly. This is indirectly testing git resource and does not add much value. (https://github.com/knative/build/tree/master/test/reuse-git-init)

why: Build supported this feature and one of the build yaml test relies on this feature what: Taskrun can template volumes names and config volume sources

bobcatfish · 2019-02-15T20:07:21Z

Really nice work @shashwathi !!! The examples dir is looking great :D :D :D

Thanks for catching and fixing those issues we'd missed as well ❤️ - and thanks for taking this on, hope it wasn't too tedious 😅

/lgtm
/meow space

knative-prow-robot · 2019-02-15T20:07:22Z

@bobcatfish:

In response to this:

Really nice work @shashwathi !!! The examples dir is looking great :D :D :D

Thanks for catching and fixing those issues we'd missed as well ❤️ - and thanks for taking this on, hope it wasn't too tedious 😅

/lgtm
/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.

googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Feb 12, 2019

knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 12, 2019

knative-prow-robot requested review from dlorenc and imjasonh February 12, 2019 17:30

nader-ziada reviewed Feb 13, 2019

View reviewed changes

bobcatfish self-assigned this Feb 13, 2019

bobcatfish reviewed Feb 13, 2019

View reviewed changes

bobcatfish mentioned this pull request Feb 13, 2019

Document migration from BuildTemplate+Build to Task+TaskRun #250

Closed

shashwathi force-pushed the build-yaml branch from c27b641 to 413f056 Compare February 13, 2019 19:33

shashwathi force-pushed the build-yaml branch 2 times, most recently from 0c9acd8 to 32b4923 Compare February 13, 2019 21:52

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

shashwathi force-pushed the build-yaml branch from 32b4923 to a22854a Compare February 13, 2019 22:40

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

shashwathi force-pushed the build-yaml branch from a22854a to 22b6f7b Compare February 13, 2019 23:45

knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 13, 2019

shashwathi force-pushed the build-yaml branch 2 times, most recently from 871f0ee to fa078a6 Compare February 14, 2019 16:48

bobcatfish reviewed Feb 14, 2019

View reviewed changes

shashwathi force-pushed the build-yaml branch from fa078a6 to d7833ba Compare February 14, 2019 23:49

shashwathi force-pushed the build-yaml branch from d7833ba to abc14d4 Compare February 15, 2019 15:22

Shash Reddy added 2 commits February 15, 2019 12:03

Add support for tempalting volume names and volume config names

161aa81

why: Build supported this feature and one of the build yaml test relies on this feature what: Taskrun can template volumes names and config volume sources

shashwathi force-pushed the build-yaml branch from abc14d4 to 161aa81 Compare February 15, 2019 17:03

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 15, 2019

knative-prow-robot merged commit 095868c into tektoncd:master Feb 15, 2019

shashwathi deleted the build-yaml branch February 15, 2019 20:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate yaml e2e test from knative/build #493

Migrate yaml e2e test from knative/build #493

shashwathi commented Feb 12, 2019 •

edited

Loading

knative-prow-robot commented Feb 12, 2019

nader-ziada left a comment

bobcatfish left a comment

bobcatfish Feb 13, 2019

bobcatfish Feb 14, 2019

shashwathi Feb 14, 2019

bobcatfish Feb 13, 2019

shashwathi Feb 13, 2019

bobcatfish Feb 14, 2019

shashwathi commented Feb 13, 2019 •

edited

Loading

shashwathi commented Feb 13, 2019

shashwathi commented Feb 13, 2019

bobcatfish Feb 14, 2019

bobcatfish commented Feb 14, 2019

shashwathi commented Feb 14, 2019

shashwathi commented Feb 15, 2019 •

edited

Loading

shashwathi commented Feb 15, 2019

bobcatfish commented Feb 15, 2019

knative-prow-robot commented Feb 15, 2019

Migrate yaml e2e test from knative/build #493

Migrate yaml e2e test from knative/build #493

Conversation

shashwathi commented Feb 12, 2019 • edited Loading

knative-prow-robot commented Feb 12, 2019

nader-ziada left a comment

Choose a reason for hiding this comment

bobcatfish left a comment

Choose a reason for hiding this comment

bobcatfish Feb 13, 2019

Choose a reason for hiding this comment

bobcatfish Feb 14, 2019

Choose a reason for hiding this comment

shashwathi Feb 14, 2019

Choose a reason for hiding this comment

bobcatfish Feb 13, 2019

Choose a reason for hiding this comment

shashwathi Feb 13, 2019

Choose a reason for hiding this comment

bobcatfish Feb 14, 2019

Choose a reason for hiding this comment

shashwathi commented Feb 13, 2019 • edited Loading

shashwathi commented Feb 13, 2019

shashwathi commented Feb 13, 2019

bobcatfish Feb 14, 2019

Choose a reason for hiding this comment

bobcatfish commented Feb 14, 2019

shashwathi commented Feb 14, 2019

shashwathi commented Feb 15, 2019 • edited Loading

shashwathi commented Feb 15, 2019

bobcatfish commented Feb 15, 2019

knative-prow-robot commented Feb 15, 2019

shashwathi commented Feb 12, 2019 •

edited

Loading

shashwathi commented Feb 13, 2019 •

edited

Loading

shashwathi commented Feb 15, 2019 •

edited

Loading