Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Run taskRun without a Task by adding TaskSpec #262

Merged
merged 4 commits into from
Nov 29, 2018
Merged

Run taskRun without a Task by adding TaskSpec #262

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f1bd35f
Add TaskSpec to taskRun to allow users to run task with one file
nader-ziada Nov 21, 2018
f2054fa
Refactor TaskRun to support processing of TaskSpec
nader-ziada Nov 22, 2018
bd3d292
updates to implementation of TaskSpec in taskRun
nader-ziada Nov 27, 2018
d0671a8
resolve conflicts with taskref now a pointer
nader-ziada Nov 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions docs/using.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,46 @@ See [the example PipelineRun](../examples/invocations/kritis-pipeline-run.yaml).
that the `Task` needs to run.
2. The `TaskRun` will also serve as a record of the history of the invocations of the
`Task`.
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@bobcatfish bobcatfish Nov 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

steps:
- name: build-and-push
image: gcr.io/kaniko-project/executor
command:
- /kaniko/executor
args:
- --destination=gcr.io/my-project/gohelloworld
```
If the TaskSpec is provided, TaskRef is not allowed.

See [the example TaskRun](../examples/invocations/run-kritis-test.yaml).

Expand Down
5 changes: 0 additions & 5 deletions pkg/apis/pipeline/v1alpha1/task_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,6 @@ type TaskSpec struct {
// +optional
Generation int64 `json:"generation,omitempty"`

// Following fields are from build spec for a Build resource.
// Sources specifies the inputs to the build.
Sources []buildv1alpha1.SourceSpec `json:"sources,omitempty"`

// Steps are the steps of the build; each step is run sequentially with the
// source mounted into /workspace.
Steps []corev1.Container `json:"steps,omitempty"`
Expand Down Expand Up @@ -151,7 +147,6 @@ type TaskList struct {

func (ts *TaskSpec) GetBuildSpec() *buildv1alpha1.BuildSpec {
return &buildv1alpha1.BuildSpec{
Sources: ts.Sources,
Steps: ts.Steps,
Volumes: ts.Volumes,
ServiceAccountName: ts.ServiceAccountName,
Expand Down
6 changes: 5 additions & 1 deletion pkg/apis/pipeline/v1alpha1/taskrun_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ var _ webhook.GenericCRD = (*TaskRun)(nil)

// TaskRunSpec defines the desired state of TaskRun
type TaskRunSpec struct {
TaskRef TaskRef `json:"taskRef"`
Trigger TaskTrigger `json:"trigger"`
// +optional
Inputs TaskRunInputs `json:"inputs,omitempty"`
Expand All @@ -46,6 +45,11 @@ type TaskRunSpec struct {
Generation int64 `json:"generation,omitempty"`
// +optional
ServiceAccount string `json:"serviceAccount,omitempty"`
// no more than one of the TaskRef and TaskSpec may be specified.
// +optional
TaskRef *TaskRef `json:"taskRef"`
//+optional
TaskSpec *TaskSpec `json:"taskSpec"`
}

// TaskRunInputs holds the input values that this task was invoked with.
Expand Down
11 changes: 8 additions & 3 deletions pkg/apis/pipeline/v1alpha1/taskrun_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,14 @@ func (ts *TaskRunSpec) Validate() *apis.FieldError {
return apis.ErrMissingField("spec")
}

// Check for TaskRef
if ts.TaskRef.Name == "" {
return apis.ErrMissingField("spec.taskref.name")
// can't have both taskRef and taskSpec at the same time
if (ts.TaskRef != nil && ts.TaskRef.Name != "") && ts.TaskSpec != nil {
return apis.ErrDisallowedFields("spec.taskref", "spec.taskspec")
}

// Check that one of TaskRef and TaskSpec is present
if (ts.TaskRef == nil || (ts.TaskRef != nil && ts.TaskRef.Name == "")) && ts.TaskSpec == nil {
return apis.ErrMissingField("spec.taskref.name", "spec.taskspec")
}

// Check for Trigger
Expand Down
55 changes: 47 additions & 8 deletions pkg/apis/pipeline/v1alpha1/taskrun_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (

"github.com/google/go-cmp/cmp"
"github.com/knative/pkg/apis"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

Expand Down Expand Up @@ -68,7 +69,7 @@ func TestTaskRun_Validate(t *testing.T) {
Name: "taskname",
},
Spec: TaskRunSpec{
TaskRef: TaskRef{
TaskRef: &TaskRef{
Name: "taskrefname",
},
Trigger: TaskTrigger{
Expand Down Expand Up @@ -98,19 +99,19 @@ func TestTaskRunSpec_Invalidate(t *testing.T) {
{
name: "invalid taskref name",
spec: TaskRunSpec{
TaskRef: TaskRef{},
TaskRef: &TaskRef{},
Trigger: TaskTrigger{
TriggerRef: TaskTriggerRef{
Type: TaskTriggerTypeManual,
},
},
},
wantErr: apis.ErrMissingField("spec.taskref.name"),
wantErr: apis.ErrMissingField("spec.taskref.name, spec.taskspec"),
},
{
name: "invalid taskref type",
spec: TaskRunSpec{
TaskRef: TaskRef{
TaskRef: &TaskRef{
Name: "taskrefname",
},
Trigger: TaskTrigger{
Expand All @@ -124,7 +125,7 @@ func TestTaskRunSpec_Invalidate(t *testing.T) {
{
name: "invalid taskref",
spec: TaskRunSpec{
TaskRef: TaskRef{
TaskRef: &TaskRef{
Name: "taskrefname",
},
Trigger: TaskTrigger{
Expand All @@ -135,7 +136,28 @@ func TestTaskRunSpec_Invalidate(t *testing.T) {
},
},
wantErr: apis.ErrMissingField("spec.trigger.triggerref.name"),
}}
},
{
name: "invalid taskref and taskspec together",
spec: TaskRunSpec{
TaskRef: &TaskRef{
Name: "taskrefname",
},
TaskSpec: &TaskSpec{
Steps: []corev1.Container{{
Name: "mystep",
Image: "myimage",
}},
},
Trigger: TaskTrigger{
TriggerRef: TaskTriggerRef{
Type: "manual",
},
},
},
wantErr: apis.ErrDisallowedFields("spec.taskspec", "spec.taskref"),
},
}

for _, ts := range tests {
t.Run(ts.name, func(t *testing.T) {
Expand All @@ -155,7 +177,7 @@ func TestTaskRunSpec_Validate(t *testing.T) {
{
name: "task trigger run type",
spec: TaskRunSpec{
TaskRef: TaskRef{
TaskRef: &TaskRef{
Name: "taskrefname",
},
Trigger: TaskTrigger{
Expand All @@ -169,7 +191,7 @@ func TestTaskRunSpec_Validate(t *testing.T) {
{
name: "task trigger run type with different capitalization",
spec: TaskRunSpec{
TaskRef: TaskRef{
TaskRef: &TaskRef{
Name: "taskrefname",
},
Trigger: TaskTrigger{
Expand All @@ -180,6 +202,23 @@ func TestTaskRunSpec_Validate(t *testing.T) {
},
},
},
{
name: "taskspec without a taskRef",
spec: TaskRunSpec{
TaskSpec: &TaskSpec{
Steps: []corev1.Container{{
Name: "mystep",
Image: "myimage",
}},
},
Trigger: TaskTrigger{
TriggerRef: TaskTriggerRef{
Type: "PiPeLiNeRuN",
Name: "testtrigger",
},
},
},
},
}

for _, ts := range tests {
Expand Down
27 changes: 18 additions & 9 deletions pkg/apis/pipeline/v1alpha1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ func (c *Reconciler) createTaskRun(logger *zap.SugaredLogger, t *v1alpha1.Task,
},
},
Spec: v1alpha1.TaskRunSpec{
TaskRef: v1alpha1.TaskRef{
TaskRef: &v1alpha1.TaskRef{
Name: t.Name,
},
Inputs: v1alpha1.TaskRunInputs{
Expand Down
4 changes: 2 additions & 2 deletions pkg/reconciler/v1alpha1/pipelinerun/pipelinerun_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,7 @@ func TestReconcile(t *testing.T) {
},
Spec: v1alpha1.TaskRunSpec{
ServiceAccount: "test-sa",
TaskRef: v1alpha1.TaskRef{
TaskRef: &v1alpha1.TaskRef{
Name: "unit-test-task",
},
Inputs: v1alpha1.TaskRunInputs{
Expand Down Expand Up @@ -471,7 +471,7 @@ func TestUpdateTaskRunsState(t *testing.T) {
},
Spec: v1alpha1.TaskRunSpec{
ServiceAccount: "test-sa",
TaskRef: v1alpha1.TaskRef{
TaskRef: &v1alpha1.TaskRef{
Name: "unit-test-task",
},
},
Expand Down
4 changes: 2 additions & 2 deletions pkg/reconciler/v1alpha1/taskrun/resources/build_step_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ func TestPostBuildSteps(t *testing.T) {
Namespace: "foo",
},
Spec: v1alpha1.TaskRunSpec{
TaskRef: v1alpha1.TaskRef{
TaskRef: &v1alpha1.TaskRef{
Name: "",
APIVersion: "a1",
},
Expand Down Expand Up @@ -129,7 +129,7 @@ func TestPreBuildSteps(t *testing.T) {
Namespace: "foo",
},
Spec: v1alpha1.TaskRunSpec{
TaskRef: v1alpha1.TaskRef{
TaskRef: &v1alpha1.TaskRef{
Name: "",
APIVersion: "a1",
},
Expand Down
Loading