Run taskRun without a Task by adding TaskSpec #262

nader-ziada · 2018-11-23T15:02:31Z

Fixes #249

Proposed Changes

Add a TaskSpec field to the TaskRun which is mutually exclusive with taskRef
validation for not having taskRef and taskSpec together but having one of them
Changed TaskRef field in taskRun to a pointer now that's optional
Unit tests for running TaskRun without a task
~~if TaskSpec includes sources it doesn't need to be defined as a Resource~~

now with this feature, users can run TaskRun without a task as demonstrated by the following example:

apiVersion: pipeline.knative.dev/v1alpha1
kind: PipelineResource
metadata:
  name: go-example-git
spec:
  type: git
  params:
  - name: url
    value: https://github.com/pivotal-nader-ziada/gohelloworld
---
apiVersion: pipeline.knative.dev/v1alpha1
kind: TaskRun
metadata:
  name: build-push-task-run-2
spec:
  trigger:
    triggerRef:
      type: manual
  inputs:
    resources:
    - name: workspace
      resourceRef:
        name: go-example-git
  taskSpec: 
    inputs:
      resources:
      - name: workspace
        type: git    
    steps:
    - name: build-and-push
      image: gcr.io/kaniko-project/executor
      command:
      - /kaniko/executor
      args:
      - --destination=gcr.io/my-project/gohelloworld

vdemeester

One comment, otherwise looks good 👼

vdemeester · 2018-11-26T10:31:20Z

pkg/reconciler/v1alpha1/taskrun/validate.go

 	}
-	return validateTaskRunAndTask(c, *tr, t, tr.Namespace)
+	return nil


Shouldn't we validate it in that case too ? 🤔

This would require to change the signature of validateTaskRunTask though, but most likely for the good

// […] return validateTaskRunAndTask(c, *tr, t.Spec, tr.Namespace) } return validateTaskRunAndTask(c, *tr, tr.TaskSpec, tr.Namespace)

(On that note, thinking outloud, we only validate the spec, so we may want to pass only the specs — the tr.Name is only required on the error message, so we could use github.com/pkg/errors to wrap errors and thus adding the tr.Name via the caller)

good point, I should add that

shashwathi

I would recommend updating taskrun test(kaniko) or pipelinerun e2e test to include a taskrun which has inlined taskspec. WDYT @pivotal-nader-ziada ?

pkg/apis/pipeline/v1alpha1/taskrun_validation.go

bobcatfish

Some comments from pair review with @pivotal-nader-ziada :

leaning toward not supporting sources in this initial PR
need some docs on how to use this

bobcatfish · 2018-11-26T18:10:56Z

pkg/reconciler/v1alpha1/taskrun/taskrun_test.go

+						Name:        "myarg",
+						Description: "mydesc",
+						Default:     "mydefault",
+					}},


looks like maybe we DO support params! i cant see a strong reason not support them esp. if we get it for free

pkg/reconciler/v1alpha1/taskrun/taskrun_test.go

nader-ziada · 2018-11-27T15:58:02Z

@bobcatfish addressed the comments, ready for another look

bobcatfish · 2018-11-28T00:03:41Z

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

+		taskSpec = taskRun.Spec.TaskSpec
+		taskName = taskRun.Name
+	} else {
+		return taskSpec, taskName, fmt.Errorf("TaskRun %s not providing TaskRef or TaskSpec", taskRun.Name)


this happens to be the only line not covered by the unit tests and i actually wonder if we need it at all - at this point, we do happen to know that the taskSpec has been validated, and there should be no case like this

however maybe its safer to keep it - im fine with not covering the line in unit tests :D

bobcatfish · 2018-11-28T00:04:23Z

Looks great to me! Thanks for adding the docs also ^.^

/lgtm

I'm going to add a hold here b/c @imjasonh opened the issue and might have some opinions about us removing sources

/hold

/meow space

knative-prow-robot · 2018-11-28T00:04:25Z

@bobcatfish:

In response to this:

Looks great to me! Thanks for adding the docs also ^.^

/lgtm

I'm going to add a hold here b/c @imjasonh opened the issue and might have some opinions about us removing sources

/hold

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

vdemeester

/lgtm

imjasonh · 2018-11-28T21:48:22Z

docs/using.md

+3. Another way of running a Task is embedding the TaskSpec in the taskRun yaml as shown in the following example
+
+```yaml
+apiVersion: pipeline.knative.dev/v1alpha1


So we still need to create two resources to run a simple set of steps? How can we drop the requirement that a PipelineResource already exists?

In Build, resources were specified directly in the build itself, as source(s).

yes, we felt this would be an acceptable compromise so its not too different from the rest of the pipeline project and confusing for users

How can we drop the requirement that a PipelineResource already exists?

One option @pivotal-nader-ziada and I talked about was embedding the Resource spec inside the resource field in the TaskRun, e.g.:

inputs: resources: - name: workspace resourceSpec: type: git params: - name: url value: https://github.com/pivotal-nader-ziada/gohelloworld

This is kinda the direction @shashwathi is going in #270 as well, where we can use the TaskRunResource to provide a path where the resource will be located in a PVC.

We'd need to actually create an instance of the Resource probably, so we can extend Resources generically (#238)

@imjasonh would you like to see something like that in this PR or a follow up PR?

I think adding even more embedding is just making the yaml file too complicated. Also one side benefit is that the resources are such a core concept in build-pipeline that we want users to know about when migrating.

Maybe as part of the work for the resources extensibility we can make this easier by making the taskRun controller create the resource for for system supported resource types such as git. In that case, the we can define types for system resources with their specific fields to simplify yaml files for all tasks

Depending on what we decide here, how big the change is, it might be better to have another follow up PR so this one doesn't get too big

I think adding even more embedding is just making the yaml file too complicated. Also one side benefit is that the resources are such a core concept in build-pipeline that we want users to know about when migrating.

That's a good point!

Depending on what we decide here, how big the change is, it might be better to have another follow up PR so this one doesn't get too big

Sounds good to me

imjasonh · 2018-11-28T21:49:31Z

docs/using.md

+    inputs:
+      resources:
+      - name: workspace
+        type: git    


This type is specified twice, once in the type of the workspace resource (line 194) and again here. Can one be removed? What happens if the resource is type:foo but I specify it as an input with type:bar ?

the runtime validation in the controller would give an error about the resource type. The reason its defined twice is that its usually in two different objects, task and taskRun, and we were trying to use the existing code and types

imjasonh · 2018-11-28T21:50:16Z

pkg/apis/pipeline/v1alpha1/taskrun_validation.go

+	// can't have both taskRef and taskSpec at the same time
+	if (ts.TaskRef != nil && ts.TaskRef.Name != "") && ts.TaskSpec != nil {
+		fields := []string{"spec.taskref", "spec.taskspec"}
+		return apis.ErrDisallowedFields(fields...)


This should work:

return apis.ErrDisallowedFields("spec.taskref", "spec.taskspec")

ok, I can change that one

imjasonh · 2018-11-28T21:50:22Z

pkg/apis/pipeline/v1alpha1/taskrun_validation.go

+
+	// Check that one of TaskRef and TaskSpec is present
+	if (ts.TaskRef == nil || (ts.TaskRef != nil && ts.TaskRef.Name == "")) && ts.TaskSpec == nil {
+		fields := []string{"spec.taskref.name", "spec.taskspec"}


will also change it

nader-ziada · 2018-11-29T14:17:55Z

/hold cancel

since I made a small fix and rebased which removed the lgtm

imjasonh · 2018-11-29T18:31:41Z

/lgtm

Sounds like there are some more streaming changes we should make, but those definitely don't belong in this PR.

Thanks for doing this work Nader, this should definitely improve the basic getting-started experience, and will make Serving usage simpler, once we get there.

- users can specifiy TaskRef or TaskSpec, but not both - update type - add validation and tests - update taskRef everywhere to be a pointer since its now optional Signed-off-by: Nader Ziada <[email protected]>

- remove support for using sources in the taskSpec - use TaskResolver - refactor tests Signed-off-by: Nader Ziada <[email protected]>

Signed-off-by: Nader Ziada <[email protected]>

knative-metrics-robot · 2018-11-29T18:54:13Z

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/taskrun_validation.go	93.1%	93.3%	0.2
pkg/reconciler/v1alpha1/taskrun/resources/taskrunresolution.go	100.0%	96.6%	-3.4
pkg/reconciler/v1alpha1/taskrun/taskrun.go	74.1%	72.2%	-1.9

vdemeester

/lgtm

knative-prow-robot · 2018-11-29T19:15:05Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

This makes it safe to remove triggers from a cluster without impacting the shared pipelines namespace. Fixes tektoncd#262.

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

knative-prow-robot requested review from aaron-prindle and imjasonh November 23, 2018 15:02

nader-ziada force-pushed the run-task branch from 1495fbe to 0959c36 Compare November 23, 2018 15:48

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 Nov 23, 2018

vdemeester reviewed Nov 26, 2018

View reviewed changes

shashwathi reviewed Nov 26, 2018

View reviewed changes

pkg/apis/pipeline/v1alpha1/taskrun_validation.go Outdated Show resolved Hide resolved

bobcatfish reviewed Nov 26, 2018

View reviewed changes

nader-ziada force-pushed the run-task branch from 0959c36 to 17de7e2 Compare November 27, 2018 15:57

nader-ziada force-pushed the run-task branch from 17de7e2 to bfc4806 Compare November 27, 2018 17:33

bobcatfish reviewed Nov 28, 2018

View reviewed changes

knative-prow-robot assigned bobcatfish Nov 28, 2018

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

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

vdemeester approved these changes Nov 28, 2018

View reviewed changes

knative-prow-robot assigned vdemeester Nov 28, 2018

imjasonh reviewed Nov 28, 2018

View reviewed changes

nader-ziada force-pushed the run-task branch from bfc4806 to 41d2531 Compare November 29, 2018 00:31

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

nader-ziada force-pushed the run-task branch from 41d2531 to 2a2733c Compare November 29, 2018 14:00

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

knative-prow-robot assigned imjasonh Nov 29, 2018

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

nader-ziada added 4 commits November 29, 2018 13:33

Add TaskSpec to taskRun to allow users to run task with one file

f1bd35f

- users can specifiy TaskRef or TaskSpec, but not both - update type - add validation and tests - update taskRef everywhere to be a pointer since its now optional Signed-off-by: Nader Ziada <[email protected]>

Refactor TaskRun to support processing of TaskSpec

f2054fa

updates to implementation of TaskSpec in taskRun

bd3d292

- remove support for using sources in the taskSpec - use TaskResolver - refactor tests Signed-off-by: Nader Ziada <[email protected]>

resolve conflicts with taskref now a pointer

d0671a8

Signed-off-by: Nader Ziada <[email protected]>

nader-ziada force-pushed the run-task branch from 2a2733c to d0671a8 Compare November 29, 2018 18:52

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

vdemeester approved these changes Nov 29, 2018

View reviewed changes

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

knative-prow-robot merged commit 3076f41 into tektoncd:master Nov 29, 2018

bobcatfish mentioned this pull request Dec 6, 2018

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

Closed

nader-ziada deleted the run-task branch February 13, 2019 14:02

mchmarny unassigned vdemeester and imjasonh Mar 7, 2019

pradeepitm12 pushed a commit to pradeepitm12/pipeline that referenced this pull request Jan 28, 2021

Remove namespace config.

e6881e2

This makes it safe to remove triggers from a cluster without impacting the shared pipelines namespace. Fixes tektoncd#262.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run taskRun without a Task by adding TaskSpec #262

Run taskRun without a Task by adding TaskSpec #262

nader-ziada commented Nov 23, 2018 •

edited

Loading

vdemeester left a comment

vdemeester Nov 26, 2018 •

edited

Loading

nader-ziada Nov 26, 2018

shashwathi left a comment

bobcatfish left a comment

bobcatfish Nov 26, 2018

nader-ziada commented Nov 27, 2018

bobcatfish Nov 28, 2018

bobcatfish commented Nov 28, 2018

knative-prow-robot commented Nov 28, 2018

vdemeester left a comment

imjasonh Nov 28, 2018

nader-ziada Nov 29, 2018

bobcatfish Nov 29, 2018 •

edited

Loading

nader-ziada Nov 29, 2018

bobcatfish Nov 29, 2018

imjasonh Nov 28, 2018

nader-ziada Nov 29, 2018

imjasonh Nov 28, 2018

nader-ziada Nov 29, 2018

imjasonh Nov 28, 2018

nader-ziada Nov 29, 2018

nader-ziada commented Nov 29, 2018

imjasonh commented Nov 29, 2018

knative-metrics-robot commented Nov 29, 2018

vdemeester left a comment

knative-prow-robot commented Nov 29, 2018

Run taskRun without a Task by adding TaskSpec #262

Run taskRun without a Task by adding TaskSpec #262

Conversation

nader-ziada commented Nov 23, 2018 • edited Loading

Proposed Changes

vdemeester left a comment

Choose a reason for hiding this comment

vdemeester Nov 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shashwathi left a comment

Choose a reason for hiding this comment

bobcatfish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nader-ziada commented Nov 27, 2018

Choose a reason for hiding this comment

bobcatfish commented Nov 28, 2018

knative-prow-robot commented Nov 28, 2018

vdemeester 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 Nov 29, 2018 • 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

nader-ziada commented Nov 29, 2018

imjasonh commented Nov 29, 2018

knative-metrics-robot commented Nov 29, 2018

vdemeester left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Nov 29, 2018

nader-ziada commented Nov 23, 2018 •

edited

Loading

vdemeester Nov 26, 2018 •

edited

Loading

bobcatfish Nov 29, 2018 •

edited

Loading