Skip to content

Commit

Permalink
Use trigger instead of triggerRef
Browse files Browse the repository at this point in the history
We use the syntax `somethingRef` in several fields on our specs because
we want to be able to use ducktyping down the line to switch out other
types (see tektoncd#238).

However `triggerRef` is a bit weird because:
- If the type is `manual`, there is no actual CRD to reference
- We don't know what other sorts of types we might need (e.g. when
  created via events)
- It was inconsistent b/w TaskRun and PipelineRun (TaskRun had an extra
  level of indirection)

This commit applies @sebgoa's suggestion to simplify this to just
`trigger`.

This was part of the discussion in tektoncd#320 about simplifying the
interfaace.
  • Loading branch information
bobcatfish committed Jan 4, 2019
1 parent a44adb0 commit 07c3a62
Show file tree
Hide file tree
Showing 20 changed files with 66 additions and 121 deletions.
22 changes: 9 additions & 13 deletions docs/tutorial.md
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,7 @@ spec:
taskRef:
name: echo-hello-world
trigger:
triggerRef:
type: manual
type: manual
```

To apply the yaml files use the following command
Expand All @@ -62,7 +61,7 @@ kind: TaskRun
metadata:
annotations:
kubectl.kubernetes.io/last-applied-configuration: |
{"apiVersion":"pipeline.knative.dev/v1alpha1","kind":"TaskRun","metadata":{"annotations":{},"name":"echo-hello-world-task-run","namespace":"default"},"spec":{"taskRef":{"name":"echo-hello-world"},"trigger":{"triggerRef":{"type":"manual"}}}}
{"apiVersion":"pipeline.knative.dev/v1alpha1","kind":"TaskRun","metadata":{"annotations":{},"name":"echo-hello-world-task-run","namespace":"default"},"spec":{"taskRef":{"name":"echo-hello-world"},"trigger":{"type":"manual"}}}
creationTimestamp: 2018-12-11T15:49:13Z
generation: 1
name: echo-hello-world-task-run
Expand All @@ -78,8 +77,7 @@ spec:
name: echo-hello-world
taskSpec: null
trigger:
triggerRef:
type: manual
type: manual
status:
conditions:
- lastTransitionTime: 2018-12-11T15:50:09Z
Expand Down Expand Up @@ -198,8 +196,7 @@ spec:
taskRef:
name: build-docker-image-from-git-source
trigger:
triggerRef:
type: manual
type: manual
inputs:
resources:
- name: workspace
Expand Down Expand Up @@ -259,7 +256,7 @@ kind: TaskRun
metadata:
annotations:
kubectl.kubernetes.io/last-applied-configuration: |
{"apiVersion":"pipeline.knative.dev/v1alpha1","kind":"TaskRun","metadata":{"annotations":{},"name":"build-docker-image-from-git-source-task-run","namespace":"default"},"spec":{"inputs":{"params":[{"name":"pathToDockerFile","value":"Dockerfile"},{"name":"pathToContext","value":"/workspace/examples/microservices/leeroy-web"}],"resources":[{"name":"workspace","resourceRef":{"name":"skaffold-git"}}]},"outputs":{"resources":[{"name":"builtImage","resourceRef":{"name":"skaffold-image-leeroy-web"}}]},"results":{"type":"gcs","url":"gcs://somebucket/results/logs"},"taskRef":{"name":"build-docker-image-from-git-source"},"trigger":{"triggerRef":{"type":"manual"}}}}
{"apiVersion":"pipeline.knative.dev/v1alpha1","kind":"TaskRun","metadata":{"annotations":{},"name":"build-docker-image-from-git-source-task-run","namespace":"default"},"spec":{"inputs":{"params":[{"name":"pathToDockerFile","value":"Dockerfile"},{"name":"pathToContext","value":"/workspace/examples/microservices/leeroy-web"}],"resources":[{"name":"workspace","resourceRef":{"name":"skaffold-git"}}]},"outputs":{"resources":[{"name":"builtImage","resourceRef":{"name":"skaffold-image-leeroy-web"}}]},"results":{"type":"gcs","url":"gcs://somebucket/results/logs"},"taskRef":{"name":"build-docker-image-from-git-source"},"trigger":{"type":"manual"}}}
creationTimestamp: 2018-12-11T18:14:29Z
generation: 1
name: build-docker-image-from-git-source-task-run
Expand Down Expand Up @@ -293,8 +290,7 @@ spec:
name: build-docker-image-from-git-source
taskSpec: null
trigger:
triggerRef:
type: manual
type: manual
status:
conditions:
- lastTransitionTime: 2018-12-11T18:15:09Z
Expand Down Expand Up @@ -416,7 +412,7 @@ metadata:
spec:
pipelineRef:
name: tutorial-pipeline
triggerRef:
trigger:
type: manual
resources:
- name: build-skaffold-web
Expand Down Expand Up @@ -462,7 +458,7 @@ kind: PipelineRun
metadata:
annotations:
kubectl.kubernetes.io/last-applied-configuration: |
{"apiVersion":"pipeline.knative.dev/v1alpha1","kind":"PipelineRun","metadata":{"annotations":{},"name":"tutorial-pipeline-run-1","namespace":"default"},"spec":{"pipelineRef":{"name":"tutorial-pipeline"},"resources":[{"inputs":[{"name":"workspace","resourceRef":{"name":"skaffold-git"}}],"name":"build-skaffold-web","outputs":[{"name":"builtImage","resourceRef":{"name":"skaffold-image-leeroy-web"}}]},{"inputs":[{"name":"workspace","resourceRef":{"name":"skaffold-git"}},{"name":"image","resourceRef":{"name":"skaffold-image-leeroy-web"}}],"name":"deploy-web"}],"triggerRef":{"type":"manual"}}}
{"apiVersion":"pipeline.knative.dev/v1alpha1","kind":"PipelineRun","metadata":{"annotations":{},"name":"tutorial-pipeline-run-1","namespace":"default"},"spec":{"pipelineRef":{"name":"tutorial-pipeline"},"resources":[{"inputs":[{"name":"workspace","resourceRef":{"name":"skaffold-git"}}],"name":"build-skaffold-web","outputs":[{"name":"builtImage","resourceRef":{"name":"skaffold-image-leeroy-web"}}]},{"inputs":[{"name":"workspace","resourceRef":{"name":"skaffold-git"}},{"name":"image","resourceRef":{"name":"skaffold-image-leeroy-web"}}],"name":"deploy-web"}],"trigger":{"type":"manual"}}}
creationTimestamp: 2018-12-11T20:30:19Z
generation: 1
name: tutorial-pipeline-run-1
Expand Down Expand Up @@ -498,7 +494,7 @@ spec:
name: deploy-web
outputs: null
serviceAccount: ""
triggerRef:
trigger:
type: manual
status:
conditions:
Expand Down
6 changes: 2 additions & 4 deletions docs/using.md
Original file line number Diff line number Diff line change
Expand Up @@ -385,8 +385,7 @@ metadata:
name: build-push-task-run-2
spec:
trigger:
triggerRef:
type: manual
type: manual
inputs:
resources:
- name: workspace
Expand Down Expand Up @@ -423,8 +422,7 @@ metadata:
spec:
sericeAccount: test-build-robot-git-ssh
trigger:
triggerRef:
type: manual
type: manual
inputs:
resources:
- name: workspace
Expand Down
2 changes: 1 addition & 1 deletion examples/run/output-pipeline-run.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ metadata:
spec:
pipelineRef:
name: output-pipeline
triggerRef:
trigger:
type: manual
serviceAccount: 'default'
results:
Expand Down
2 changes: 1 addition & 1 deletion examples/run/pipeline-run.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ metadata:
spec:
pipelineRef:
name: demo-pipeline
triggerRef:
trigger:
type: manual
serviceAccount: 'default'
results:
Expand Down
3 changes: 1 addition & 2 deletions examples/run/task-run.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,7 @@ spec:
taskRef:
name: build-push
trigger:
triggerRef:
type: manual
type: manual
results:
type: 'gcs'
url: 'gcs://somebucket/results/logs'
Expand Down
6 changes: 3 additions & 3 deletions pkg/apis/pipeline/v1alpha1/pipelinerun_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ var _ webhook.GenericCRD = (*TaskRun)(nil)
// PipelineRunSpec defines the desired state of PipelineRun
type PipelineRunSpec struct {
PipelineRef PipelineRef `json:"pipelineRef"`
PipelineTriggerRef PipelineTriggerRef `json:"triggerRef"`
PipelineTrigger PipelineTrigger `json:"trigger"`
PipelineTaskResources []PipelineTaskResource `json:"resources"`
// +optional
ServiceAccount string `json:"serviceAccount"`
Expand Down Expand Up @@ -92,9 +92,9 @@ const (
PipelineTriggerTypeManual PipelineTriggerType = "manual"
)

// PipelineTriggerRef describes what triggered this Pipeline to run. It could be triggered manually,
// PipelineTrigger describes what triggered this Pipeline to run. It could be triggered manually,
// or it could have been some kind of external event (not yet designed).
type PipelineTriggerRef struct {
type PipelineTrigger struct {
Type PipelineTriggerType `json:"type"`
// +optional
Name string `json:"name,omitempty"`
Expand Down
4 changes: 2 additions & 2 deletions pkg/apis/pipeline/v1alpha1/pipelinerun_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@ func (ps *PipelineRunSpec) Validate() *apis.FieldError {
if ps.PipelineRef.Name == "" {
return apis.ErrMissingField("pipelinerun.spec.Pipelineref.Name")
}
if ps.PipelineTriggerRef.Type != PipelineTriggerTypeManual {
return apis.ErrInvalidValue(string(ps.PipelineTriggerRef.Type), "pipelinerun.spec.triggerRef.type")
if ps.PipelineTrigger.Type != PipelineTriggerTypeManual {
return apis.ErrInvalidValue(string(ps.PipelineTrigger.Type), "pipelinerun.spec.trigger.type")
}
// check for results
if ps.Results != nil {
Expand Down
8 changes: 4 additions & 4 deletions pkg/apis/pipeline/v1alpha1/pipelinerun_validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ func TestPipelineRun_Invalidate(t *testing.T) {
Name: "pipelinelineName",
},
Spec: PipelineRunSpec{
PipelineTriggerRef: PipelineTriggerRef{
PipelineTrigger: PipelineTrigger{
Type: PipelineTriggerTypeManual,
},
},
Expand All @@ -72,12 +72,12 @@ func TestPipelineRun_Invalidate(t *testing.T) {
PipelineRef: PipelineRef{
Name: "prname",
},
PipelineTriggerRef: PipelineTriggerRef{
PipelineTrigger: PipelineTrigger{
Type: "badtype",
},
},
},
want: apis.ErrInvalidValue("badtype", "pipelinerun.spec.triggerRef.type"),
want: apis.ErrInvalidValue("badtype", "pipelinerun.spec.trigger.type"),
},
}

Expand All @@ -100,7 +100,7 @@ func TestPipelineRun_Validate(t *testing.T) {
PipelineRef: PipelineRef{
Name: "prname",
},
PipelineTriggerRef: PipelineTriggerRef{
PipelineTrigger: PipelineTrigger{
Type: "manual",
},
Results: &Results{
Expand Down
9 changes: 2 additions & 7 deletions pkg/apis/pipeline/v1alpha1/taskrun_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,11 +69,6 @@ type TaskRunOutputs struct {
Params []Param `json:"params,omitempty"`
}

// TaskTrigger defines a webhook style trigger to start a TaskRun
type TaskTrigger struct {
TriggerRef TaskTriggerRef `json:"triggerRef"`
}

// TaskTriggerType indicates the mechanism by which this TaskRun was created.
type TaskTriggerType string

Expand All @@ -87,10 +82,10 @@ const (
TaskTriggerTypePipelineRun TaskTriggerType = "pipelineRun"
)

// TaskTriggerRef describes what triggered this Task to run. It could be triggered manually,
// TaskTrigger describes what triggered this Task to run. It could be triggered manually,
// or it may have been part of a PipelineRun in which case this ref would refer
// to the corresponding PipelineRun.
type TaskTriggerRef struct {
type TaskTrigger struct {
Type TaskTriggerType `json:"type"`
// +optional
Name string `json:"name,omitempty"`
Expand Down
6 changes: 4 additions & 2 deletions pkg/apis/pipeline/v1alpha1/taskrun_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ func (ts *TaskRunSpec) Validate() *apis.FieldError {
}

// Check for Trigger
if err := ts.Trigger.TriggerRef.Validate("spec.trigger.triggerref"); err != nil {
if err := ts.Trigger.Validate("spec.trigger"); err != nil {
return err
}

Expand Down Expand Up @@ -96,7 +96,9 @@ func checkForPipelineResourceDuplicates(resources []TaskResourceBinding, path st
return nil
}

func (r TaskTriggerRef) Validate(path string) *apis.FieldError {
// Validate validates that the task trigger is of a known type. If it was triggered by a PipelineRun, the
// name of the trigger should be the name of a PipelienRun.
func (r TaskTrigger) Validate(path string) *apis.FieldError {
if r.Type == "" {
return nil
}
Expand Down
50 changes: 17 additions & 33 deletions pkg/apis/pipeline/v1alpha1/taskrun_validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,10 +73,8 @@ func TestTaskRun_Validate(t *testing.T) {
Name: "taskrefname",
},
Trigger: TaskTrigger{
TriggerRef: TaskTriggerRef{
Type: TaskTriggerTypePipelineRun,
Name: "testtriggername",
},
Type: TaskTriggerTypePipelineRun,
Name: "testtriggername",
},
},
}
Expand All @@ -101,41 +99,35 @@ func TestTaskRunSpec_Invalidate(t *testing.T) {
spec: TaskRunSpec{
TaskRef: &TaskRef{},
Trigger: TaskTrigger{
TriggerRef: TaskTriggerRef{
Type: TaskTriggerTypeManual,
},
Type: TaskTriggerTypeManual,
},
},
wantErr: apis.ErrMissingField("spec.taskref.name, spec.taskspec"),
},
{
name: "invalid taskref type",
name: "invalid trigger type",
spec: TaskRunSpec{
TaskRef: &TaskRef{
Name: "taskrefname",
},
Trigger: TaskTrigger{
TriggerRef: TaskTriggerRef{
Type: "wrongtype",
},
Type: "wrongtype",
},
},
wantErr: apis.ErrInvalidValue("wrongtype", "spec.trigger.triggerref.type"),
wantErr: apis.ErrInvalidValue("wrongtype", "spec.trigger.type"),
},
{
name: "invalid taskref",
name: "triggered by pipelinerun without name",
spec: TaskRunSpec{
TaskRef: &TaskRef{
Name: "taskrefname",
},
Trigger: TaskTrigger{
TriggerRef: TaskTriggerRef{
Type: TaskTriggerTypePipelineRun,
Name: "",
},
Type: TaskTriggerTypePipelineRun,
Name: "",
},
},
wantErr: apis.ErrMissingField("spec.trigger.triggerref.name"),
wantErr: apis.ErrMissingField("spec.trigger.name"),
},
{
name: "invalid taskref and taskspec together",
Expand All @@ -150,9 +142,7 @@ func TestTaskRunSpec_Invalidate(t *testing.T) {
}},
},
Trigger: TaskTrigger{
TriggerRef: TaskTriggerRef{
Type: "manual",
},
Type: "manual",
},
},
wantErr: apis.ErrDisallowedFields("spec.taskspec", "spec.taskref"),
Expand Down Expand Up @@ -181,10 +171,8 @@ func TestTaskRunSpec_Validate(t *testing.T) {
Name: "taskrefname",
},
Trigger: TaskTrigger{
TriggerRef: TaskTriggerRef{
Type: TaskTriggerTypePipelineRun,
Name: "testtrigger",
},
Type: TaskTriggerTypePipelineRun,
Name: "testtrigger",
},
Results: &Results{
URL: "http://www.google.com",
Expand All @@ -199,10 +187,8 @@ func TestTaskRunSpec_Validate(t *testing.T) {
Name: "taskrefname",
},
Trigger: TaskTrigger{
TriggerRef: TaskTriggerRef{
Type: "PiPeLiNeRuN",
Name: "testtrigger",
},
Type: "PiPeLiNeRuN",
Name: "testtrigger",
},
},
},
Expand All @@ -216,10 +202,8 @@ func TestTaskRunSpec_Validate(t *testing.T) {
}},
},
Trigger: TaskTrigger{
TriggerRef: TaskTriggerRef{
Type: "PiPeLiNeRuN",
Name: "testtrigger",
},
Type: "PiPeLiNeRuN",
Name: "testtrigger",
},
},
},
Expand Down
Loading

0 comments on commit 07c3a62

Please sign in to comment.