Declare resources in pipeline 😇 #415

bobcatfish · 2019-01-22T00:33:06Z

Now resources will be bound to a "name" which is used in the Pipeline
to refer to them. This will simplify the binding declarations in
PipelineRun and also make the Pipeline definition solely responsible
for linking resources to Tasks.

This will let users effectively say something like:

“For the Pipeline, use git resource W, image resources Y + X, and deploy to cluster Z.”

Instead of previously, where it was more like:

“For Task A use git resource W and image X, for Task B use git resource W and image Y, for Task C use git resource W and image Y and image X and cluster Z…”

This also has some side benefits in allowing us to do some extra validation at creation time for Pipelines.

Also includes:

Updating the creation time validation of Pipelines to verify that providedBy constraints make sense (i.e. the order can be satisfied) - since we only have linear execution, this meanscanTaskRun is no longer needed 😇
Moving some docs out of concepts.md (keeping this high level) and into using.md (putting the nitty gritty in here)

Question for reviewers:

Should we actually have a type at all in the declared resources in the Pipeline? @sebgoa had suggested removing in the type, in which case we could just declare a list of names of resources. At the moment this doesn't really give us a lot in the way of extra validation b/c we'd still have to fetch Tasks to see if the types match up, which we only do at runtime. I do feel like there is value in telling the user what types these should be so they know what to bind though 🤔

I think the end to end and sample tests may fail but I wanted to get this PR out and start the review process asap! 😅

(This fixes most of #320! 🎉 )

p.s. sorry this is so huge! couldn't figure out how to break it up 😅

bobcatfish · 2019-01-22T00:47:34Z

bobcatfish · 2019-01-22T00:48:52Z

I0122 00:44:20.781] --- SKIP: TestStorageTaskRun (0.00s)
I0122 00:44:20.781]     gcs_taskrun_test.go:40: KANIKO_SECRET_CONFIG_FILE variable is not set.

@shashwathi is that test supposed to get skipped in CI?

vdemeester · 2019-01-22T10:16:20Z

pkg/apis/pipeline/v1alpha1/pipeline_validation.go

+
+	// providedBy should match future tasks
+	// TODO(#168) when pipelines don't just execute linearly this will need to be more sophisticated
+	for i, t := range ps.Tasks {


We may want to extract that (or part of it 👼). Like validateProvidedBy or something.

oh yeah, that's a good call!

i think for now ill leave the testing at the level of the Validate function 🤔 kinda wondering if we should be testing these functions directly instead 🤔🤔🤔

nice this makes it much easier to have meaningful errors :D

vdemeester · 2019-01-22T13:00:49Z

docs/using.md


-We express this ordering by adding `providedBy` on `Resources` that our `Tasks`
+We express this dependency by adding `providedBy` on `Resources` that our `Tasks`


"This dependency is expressed by …" (if we want to avoid the use of we – #k8s-docs-guidelines)

whoops, this is gonna be a hard habit to break for us me 😅

maybe "Express this dependency by adding..." - avoiding passive voice? :D

vdemeester · 2019-01-22T13:01:09Z

docs/using.md

-You can see that the `builtImage` output from `build-skaffold-app` is bound to
-the `skaffold-image-leeroy-app` `PipelineResource`, and the same
-`PipelineResource` is bound to `image` for `deploy-app`.
+The `my-image` resource is expected to be provided to the `deploy-app` `Task` from


"The resource my-image …"

vdemeester · 2019-01-22T13:01:50Z

docs/using.md

-### Resource shared between tasks
+##### Configure Entrypoint image
+
+To run a step needs to pull an `Entrypoint` image. Maybe the image is hard to


I think something is missing in the first sentence

lol whoops 😅

vdemeester · 2019-01-22T13:02:12Z

docs/using.md

-mounted under path `/pvc`.
+### Resource sharing between tasks
+
+Pipeline Tasks are allowed to pass resources from previous tasks via the


s/tasks/Tasks/

vdemeester · 2019-01-22T13:07:25Z

pkg/apis/pipeline/v1alpha1/pipeline_validation.go

+}
+
+func validateDeclaredResources(ps *PipelineSpec) error {
+	needed := []string{}


s/needed/required/g ?

now these lines are exactly the same length, very satisfying 😎

required := []string{"elsa", "anna", "olaf", "kristoff"} provided := []string{"elsa", "anna", "olaf", "kristoff"}

vdemeester · 2019-01-22T13:10:02Z

pkg/apis/pipeline/v1alpha1/pipeline_validation_test.go

 			fields: fields{
+				Resources: []PipelineDeclaredResource{{


We're gonna need builders for those 🕺

i actually did add builders for these! pipeline_validation_test isn't using any builders cuz it's doing this odd (where odd = different from all the other tests) thing where it's declaring "fields" and using those to instantiate a Pipeline 🤔

ill see what i can do!

vdemeester · 2019-01-22T13:11:16Z

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

+func GetResourcesFromBindings(p *v1alpha1.Pipeline, pr *v1alpha1.PipelineRun) (map[string]v1alpha1.PipelineResourceRef, error) {
+	resources := map[string]v1alpha1.PipelineResourceRef{}
+
+	needed := make([]string, 0, len(p.Spec.Resources))


Same s/needed/required/g ?

tejal29

Can we move all the documentation changes that are not relevant to this change in another PR?

I know you mention it in "Also includes" however, i feel reviewers give more attention to code review then docs and it might be lost. Also will make review easier.

bobcatfish · 2019-01-22T19:36:47Z

Can we move all the documentation changes that are not relevant to this change in another PR?

kk i'll give it a shot!

bobcatfish · 2019-01-22T19:43:09Z

Can we move all the documentation changes that are not relevant to this change in another PR?
I know you mention it in "Also includes" however, i feel reviewers give more attention to code review then docs and it might be lost. Also will make review easier.

Actually on second thought @tejal29 how strongly do you feel about this? It's going to be a bit hard to break up, and @vdemeester has already added a bunch of comments on the doc changes in this PR.

The "PipelineResources in a Pipeline" section in using.md and the examples in docs/tutorial.md are the most relevant to the functionality that's changing in this PR, so:

If a reviewer want to focus on the doc changes, they could look just at the .md files and be aware of those sections being new
If a reviewer wants to focus only on the API changes, just look at the "PipelineResources in a Pipeline" section in using.md and the examples in docs/tutorial.md, otherwise ignore the .md files

I'll try to break it up if you feel really strongly but it's gonna be tough - I'll try to be more aware in the future to create separate PRs.

nader-ziada

looks good to me! great job @bobcatfish

nader-ziada · 2019-01-22T19:54:53Z

are you planning on doing the other items in #302 in a different PR?

nader-ziada · 2019-01-22T20:14:41Z

not sure why the test is not passing, I going to run it again to see if this was a flake

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

shashwathi

Great PR. I have suggested couple of missing test cases and nil pointer check. Rest looks awesome! 👍 🎆

I am stressing unit test cases because we are relying on this to catch most use cases. (I just noticed my suggested test cases have "crazy" in them. Please change that :D )

shashwathi · 2019-01-22T19:24:32Z

pkg/apis/pipeline/v1alpha1/pipeline_types.go

+// should be provided to a Task as its inputs and outputs.
+type PipelineTaskResources struct {
+	// Inputs holds the mapping from the PipelineResources declared in
+	// DeclaredPipelineResources to the input PipelineResources reuqired by the Task.


typo: reuqired in both Inputs, Outputs

shashwathi · 2019-01-22T20:03:39Z

pkg/apis/pipeline/v1alpha1/pipeline_validation.go

@@ -32,6 +33,39 @@ func (p *Pipeline) Validate() *apis.FieldError {
 	return nil
 }

+func isOutput(task PipelineTask, resource string) bool {
+	for _, output := range task.Resources.Outputs {


task.Resources is pointer. We need to check here if its nil. An example test case

{ name: "providedBy resource isn't present in task", fields: fields{ Resources: []PipelineDeclaredResource{{ Name: "wonderful-resource", Type: "image", }}, Tasks: []PipelineTask{{ Name: "bar", // no resources used }, { Name: "foo", Resources: &PipelineTaskResources{ Inputs: []PipelineTaskInputResource{{ Name: "wow-image", Resource: "wonderful-resource", ProvidedBy: []string{"bar"}, }}, }, }}, }, }

task.Resources is pointer

yeah i missed that a couple of times in my earlier iterations :S definitely had some panics 😅

An example test case

hmmm technically the "no duplicate tasks" test case cover this but I'm cool with adding another case explicitly!

ProvidedBy: []string{"bar"},

gonna remove that tho since that would mean bar would have to have an output

shashwathi · 2019-01-22T20:20:40Z

pkg/apis/pipeline/v1alpha1/pipeline_validation_test.go

+			},
+		},
+		{
+			name: "providedBy resource isn't output by task",


Can you also add a test case for 2nd task referring resource from non existent task? (found variable should catch that use case

{ name: "providedBy resource refers non existent task name", fields: fields{ Resources: []PipelineDeclaredResource{{ Name: "wonderful-resource", Type: "git", }}, Tasks: []PipelineTask{{ Name: "bar", }, { Name: "foo", Resources: &PipelineTaskResources{ Inputs: []PipelineTaskInputResource{{ Name: "wow-image", Resource: "wonderful-resource", ProvidedBy: []string{"bar-crazy"}, }}, }, }}, }, }

i think this is caught by providedby task doesnt exist?

providedby task doesnt exist test has resource dependency on a task that does not exist but it is the first task. The code path in this test case is

if i == 0 { .. "first task cant depend on anything" }

The test I have given above has 2 tasks and 2nd task refers a dependency that does not exist. It takes diff code paths in pipeline validation. In the code there is found variable which is suppose to catch this test case.

ohhh okay I think I finally see what you're saying!

sorry I was so slow to catch on, and thanks for noticing this!

shashwathi · 2019-01-22T20:34:37Z

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

@@ -149,7 +163,11 @@ func ResolvePipelineRun(getTask resources.GetTask, getClusterTask resources.GetC
 		}

 		// Get all the resources that this task will be using, if any
-		inputs, outputs := getPipelineRunTaskResources(pt.Name, pr)
+		inputs, outputs, err := getPipelineRunTaskResources(pt, providedResources)
+		if err != nil {


I am guessing that error code path is defined to have clean code but we do not expect to be in this code path right?
there are couple tests cases in pipeline task resources that are missing(like missing input/output resource for task) but I can understand that we do not expect to be here since webhook catch it way before. It makes sense

yeah exactly! i have pretty mixed feelings about it, but i thought it was better to err on the safer side, just in case validation was changed later on to not catch this (would be very hard to remember to change logic here as a result!)

i see what you mean tho, it feels weird not to have test coverage for this! ill try adding it and see how we feel about it

shashwathi · 2019-01-22T20:57:20Z

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

+						Name:       "awesome-thing",
+						ProvidedBy: []string{"quest"},
+					}},
+				}},


I am unable to tag the test in this comment: TestValidateProvidedBy_Invalid test is missing a case when ResolvedTaskResourceInput refer non existent resource

{ name: "resolved task resource refers to non existent input resource ", state: []*ResolvedPipelineRunTask{{ PipelineTask: &v1alpha1.PipelineTask{ Name: "winning", Resources: &v1alpha1.PipelineTaskResources{ Inputs: []v1alpha1.PipelineTaskInputResource{{ Name: "sweet-artifact", ProvidedBy: []string{"quest"}, }}, }}, ResolvedTaskResources: tb.ResolvedTaskResources( tb.ResolvedTaskResourcesTaskSpec( tb.TaskInputs(tb.InputsResource("sweet-artifact", v1alpha1.PipelineResourceTypeImage)), ), tb.ResolvedTaskResourcesInputs("sweet-artifact-crazy", r), // non existent resource ), }}, errContains: "declare dependency", }

hm this is weird, i feel like this is another case that shouldnt actually happen - now that "providedBy" must be on inputs and its not possible to accidentally add it to outputs 🤔 ill try to add a test case for it tho!

after thinking about it more i really dont think this if statement makes sense anymore and im going to remove it - thanks for catching this @shashwathi !!

shashwathi · 2019-01-22T20:59:40Z

pkg/reconciler/v1alpha1/taskrun/list/diff.go

+// contain the same values.
+func IsSame(needed, provided []string) error {
+	missing := DiffLeft(needed, provided)
+	if len(missing) > 0 {


Probably another implementation would be to sort needed and provided array and pass them to cmp lib. Only benefit I can think of suggested approach is cmp checks both length and content of array. Just a suggestion. I don't have strong opinion about this

makes sense to me! do you mind if we table this for a future PR?

bobcatfish · 2019-01-22T22:20:14Z

Can we move all the documentation changes that are not relevant to this change in another PR?

I keep thinking about this and realizing what a good suggestion this is - I should have done this from the start @tejal29 ! I could have gotten a lot of those doc changes up for review way sooner 🤦‍♀️ Next time for sure!

bobcatfish · 2019-01-23T01:18:55Z

are you planning on doing the other items in #302 in a different PR?

@pivotal-nader-ziada yes! i want to tackle the from change in another PR, and then I'll probably create a separate issue for adding runAfter which we can tackle separately

not sure why the test is not passing, I going to run it again to see if this was a flake

boo I guess it's not a flake :)

bobcatfish · 2019-01-23T01:19:20Z

I am stressing unit test cases because we are relying on this to catch most use cases

Definitely appreciated @shashwathi 🙏 ❤️ !!

Now resources will be bound to a "name" which is used in the `Pipeline` to refer to them. This will simplify the binding declarations in `PipelineRun` and also make the `Pipeline` definition solely responsible for linking resources to Tasks. Toward tektoncd#320

bobcatfish · 2019-01-23T02:02:46Z

boo I guess it's not a flake :)

Nice, there's actually a pretty good error message :D

    message: 'PipelineRun arendelle-ptetjjtc/helm-deploy-pipeline-run doesn''t bind
      Pipeline arendelle-ptetjjtc/helm-deploy-pipeline''s PipelineResources correctly:
      PipelineRun bound resources didn''t match Pipeline: Didn''t provide needed values:
      [git-repo]'

bobcatfish · 2019-01-23T02:07:03Z

Okay should be ready for another look @shashwathi @pivotal-nader-ziada @vdemeester @tejal29 !

We'll see if I fixed the e2e test 🤞

googlebot · 2019-01-23T23:23:47Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

bobcatfish · 2019-01-23T23:24:34Z

shashwathi

/lgtm

knative-prow-robot · 2019-01-23T23:28:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish, 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~~ [bobcatfish,shashwathi]

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

bobcatfish · 2019-01-23T23:28:23Z

test/helm_task_test.go

@@ -155,7 +156,7 @@ func getCreateImageTask(namespace string, t *testing.T) *v1alpha1.Task {
 	}

 	imageName = fmt.Sprintf("%s/%s", dockerRepo, AppendRandomString(sourceImageName))
-	t.Logf("Image to be pusblished: %s", imageName)
+	logger.Infof("Image to be pusblished: %s", imageName)


o no @jonjohnsonjr would be very upset about this

shashwathi · 2019-01-23T23:56:18Z

@bobcatfish : Looks like yaml tests are failing.

googlebot · 2019-01-24T00:03:24Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

bobcatfish · 2019-01-24T00:03:36Z

@bobcatfish : Looks like yaml tests are failing.

whoops, thanks @shashwathi ! looks like I completely missed updating the output yaml examples 😅

googlebot · 2019-01-24T00:03:51Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

knative-metrics-robot · 2019-01-24T00:04:51Z

The following is the coverage report on pkg/.
Say /test pull-knative-build-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/pipeline_validation.go	75.0%	91.7%	16.7
pkg/reconciler/v1alpha1/pipeline/resources/dag.go	97.7%	97.8%	0.1
pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go	76.5%	81.3%	4.8
pkg/reconciler/v1alpha1/pipelinerun/resources/input_output_steps.go	95.5%	95.2%	-0.2
pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go	92.4%	93.0%	0.7
pkg/reconciler/v1alpha1/taskrun/validate_resources.go	98.1%	98.0%	-0.1

shashwathi · 2019-01-24T00:05:29Z

/lgtm

shashwathi · 2019-01-24T00:34:34Z

Lets have a party to celebrate this PR getting merged! 🎆 Great work @bobcatfish

nader-ziada · 2019-01-24T03:07:32Z

awesome job on this PR @bobcatfish

bobcatfish · 2019-01-24T04:20:45Z

Thanks @shashwathi and @pivotal-nader-ziada ! Thanks to everyone's careful reviewing!! @shashwathi you caught a lot of things I missed, thanks!! 🤗

knative-prow-robot requested review from dlorenc and srinivashegde86 January 22, 2019 00:33

knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 22, 2019

bobcatfish force-pushed the declare_resources_in_pipeline branch from fd6472f to b3262b7 Compare January 22, 2019 00:34

bobcatfish changed the title ~~Declare resources in pipeline~~ Declare resources in pipeline 😇 Jan 22, 2019

bobcatfish mentioned this pull request Jan 22, 2019

Reduce PipelineResource bindings #320

Closed

bobcatfish requested review from shashwathi, vdemeester and nader-ziada and removed request for dlorenc and srinivashegde86 January 22, 2019 00:39

vdemeester reviewed Jan 22, 2019

View reviewed changes

tejal29 reviewed Jan 22, 2019

View reviewed changes

nader-ziada reviewed Jan 22, 2019

View reviewed changes

shashwathi suggested changes Jan 22, 2019

View reviewed changes

bobcatfish force-pushed the declare_resources_in_pipeline branch from a61fb48 to 104a2cd Compare January 23, 2019 02:01

bobcatfish force-pushed the declare_resources_in_pipeline branch from 104a2cd to 7b10398 Compare January 23, 2019 02:06

googlebot added the cla: no label Jan 23, 2019

bobcatfish added cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit and removed cla: no labels Jan 23, 2019

knative-prow-robot assigned shashwathi Jan 23, 2019

shashwathi approved these changes Jan 23, 2019

View reviewed changes

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

bobcatfish commented Jan 23, 2019

View reviewed changes

Update output examples to use new API (tektoncd#320)

c0f80e9

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 24, 2019

googlebot added cla: no and removed cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit labels Jan 24, 2019

bobcatfish added cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit and removed cla: no labels Jan 24, 2019

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

knative-prow-robot merged commit d34c72d into tektoncd:master Jan 24, 2019

This was referenced Jan 25, 2019

Fix flaky unit tests in resources package #438

Merged

Add ability to embed Resource declaration also when embedding Tasks in TaskRuns #313

Closed

mchmarny unassigned shashwathi Mar 7, 2019


		We express this ordering by adding `providedBy` on `Resources` that our `Tasks`
		We express this dependency by adding `providedBy` on `Resources` that our `Tasks`

Declare resources in pipeline 😇 #415

Declare resources in pipeline 😇 #415

Conversation

bobcatfish commented Jan 22, 2019 • edited Loading

bobcatfish commented Jan 22, 2019

bobcatfish commented Jan 22, 2019

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

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

tejal29 left a comment • edited Loading

Choose a reason for hiding this comment

bobcatfish commented Jan 22, 2019

bobcatfish commented Jan 22, 2019

nader-ziada left a comment

Choose a reason for hiding this comment

nader-ziada commented Jan 22, 2019

nader-ziada commented Jan 22, 2019

shashwathi 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

Choose a reason for hiding this comment

bobcatfish Jan 23, 2019 • 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

shashwathi Jan 22, 2019 • 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

bobcatfish commented Jan 22, 2019

bobcatfish commented Jan 23, 2019

bobcatfish commented Jan 23, 2019

bobcatfish commented Jan 23, 2019

bobcatfish commented Jan 23, 2019

googlebot commented Jan 23, 2019

bobcatfish commented Jan 23, 2019

shashwathi left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Jan 23, 2019

Choose a reason for hiding this comment

shashwathi commented Jan 23, 2019

googlebot commented Jan 24, 2019

bobcatfish commented Jan 24, 2019

googlebot commented Jan 24, 2019

knative-metrics-robot commented Jan 24, 2019

shashwathi commented Jan 24, 2019

shashwathi commented Jan 24, 2019

nader-ziada commented Jan 24, 2019

bobcatfish commented Jan 24, 2019

bobcatfish commented Jan 22, 2019 •

edited

Loading

tejal29 left a comment •

edited

Loading

shashwathi left a comment •

edited

Loading

bobcatfish Jan 23, 2019 •

edited

Loading

shashwathi Jan 22, 2019 •

edited

Loading