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.

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

adding finally type (without implementation) at the pipeline level #2650

Merged
merged 1 commit into from
Jun 8, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
29 changes: 29 additions & 0 deletions docs/pipelines.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ weight: 3
- [Configuring execution results at the `Pipeline` level](#configuring-execution-results-at-the-pipeline-level)
- [Configuring the `Task` execution order](#configuring-the-task-execution-order)
- [Adding a description](#adding-a-description)
- [Adding `Finally` to the `Pipeline` (Preview)](#adding-finally-to-the-pipeline-preview)
Copy link
Collaborator

Choose a reason for hiding this comment

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

ooo i like calling it a "preview" good call :D

- [Code examples](#code-examples)

## Overview
Expand Down Expand Up @@ -529,6 +530,34 @@ In particular:

The `description` field is an optional field and can be used to provide description of the `Pipeline`.

## Adding `Finally` to the `Pipeline` (Preview)

_Finally type is available in the `Pipeline` but functionality is in progress. Final tasks are can be specified and
are validated but not executed yet._

You can specify a list of one or more final tasks under `finally` section. Final tasks are guaranteed to be executed
in parallel after all `PipelineTasks` under `tasks` have completed regardless of success or error. Final tasks are very
similar to `PipelineTasks` under `tasks` section and follow the same syntax. Each final task must have a
[valid](https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names) `name` and a [taskRef or
taskSpec](taskruns.md#specifying-the-target-task). For example:

```yaml
spec:
tasks:
- name: tests
taskRef:
Name: integration-test
finally:
- name: cleanup-test
taskRef:
Name: cleanup
```

_[PR #2661](https://github.com/tektoncd/pipeline/pull/2661) is implementing this new functionality by adding support to enable
final tasks along with workspaces and parameters. `PipelineRun` status is being updated to include execution status of
final tasks i.e. `PipelineRun` status is set to success or failure depending on execution of `PipelineTasks`, this status
remains same when all final tasks finishes successfully but is set to failure if any of the final tasks fail._

## Code examples

For a better understanding of `Pipelines`, study [our code examples](https://github.com/tektoncd/pipeline/tree/master/examples).
Expand Down
2 changes: 2 additions & 0 deletions pkg/apis/pipeline/v1alpha1/conversion_error.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ const (
// resources when they cannot be converted to warn of a forthcoming
// breakage.
ConditionTypeConvertible apis.ConditionType = v1beta1.ConditionTypeConvertible
// Conversion Error message for a field not available in v1alpha1
ConversionErrorFieldNotAvailableMsg = "the specified field/section is not available in v1alpha1"
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice!

)

// CannotConvertError is returned when a field cannot be converted.
Expand Down
7 changes: 7 additions & 0 deletions pkg/apis/pipeline/v1alpha1/pipeline_conversion.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ import (
"knative.dev/pkg/apis"
)

const FinallyFieldName = "finally"
afrittoli marked this conversation as resolved.
Show resolved Hide resolved

var _ apis.Convertible = (*Pipeline)(nil)

// ConvertTo implements api.Convertible
Expand All @@ -51,6 +53,7 @@ func (source *PipelineSpec) ConvertTo(ctx context.Context, sink *v1beta1.Pipelin
}
}
}
sink.Finally = nil
return nil
}

Expand Down Expand Up @@ -97,6 +100,10 @@ func (sink *PipelineSpec) ConvertFrom(ctx context.Context, source v1beta1.Pipeli
}
}
}
// finally clause was introduced in v1beta1 and not available in v1alpha1
if len(source.Finally) > 0 {
return ConvertErrorf(FinallyFieldName, ConversionErrorFieldNotAvailableMsg)
}
return nil
}

Expand Down
119 changes: 98 additions & 21 deletions pkg/apis/pipeline/v1alpha1/pipeline_conversion_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,13 +42,12 @@ func TestPipelineConversionBadType(t *testing.T) {
}
}

func TestPipelineConversion(t *testing.T) {
func TestPipelineConversion_Success(t *testing.T) {
versions := []apis.Convertible{&v1beta1.Pipeline{}}

tests := []struct {
name string
in *Pipeline
wantErr bool
pritidesai marked this conversation as resolved.
Show resolved Hide resolved
name string
in *Pipeline
}{{
name: "simple conversion",
in: &Pipeline{
Expand Down Expand Up @@ -114,7 +113,38 @@ func TestPipelineConversion(t *testing.T) {
}},
},
},
}, {
}}

for _, test := range tests {
for _, version := range versions {
t.Run(test.name, func(t *testing.T) {
ver := version
// convert v1alpha1 Pipeline to v1beta1 Pipeline
if err := test.in.ConvertTo(context.Background(), ver); err != nil {
t.Errorf("ConvertTo() = %v", err)
}
got := &Pipeline{}
// converting it back to v1alpha1 pipeline and storing it in got variable to compare with original input
if err := got.ConvertFrom(context.Background(), ver); err != nil {
t.Errorf("ConvertFrom() = %v", err)
}
// compare origin input and roundtrip Pipeline i.e. v1alpha1 pipeline converted to v1beta1 and then converted back to v1alpha1
// this check is making sure that we do not end up with different object than what we started with
if d := cmp.Diff(test.in, got); d != "" {
t.Errorf("roundtrip %s", diff.PrintWantGot(d))
pritidesai marked this conversation as resolved.
Show resolved Hide resolved
}
})
}
}
}

func TestPipelineConversion_Failure(t *testing.T) {
versions := []apis.Convertible{&v1beta1.Pipeline{}}

tests := []struct {
Copy link
Member

Choose a reason for hiding this comment

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

NIT: this is just a single test so maybe we don't need the loop

name string
in *Pipeline
}{{
name: "simple conversion with task spec error",
in: &Pipeline{
ObjectMeta: metav1.ObjectMeta{
Expand Down Expand Up @@ -152,29 +182,76 @@ func TestPipelineConversion(t *testing.T) {
}},
},
},
wantErr: true,
}}

for _, test := range tests {
for _, version := range versions {
t.Run(test.name, func(t *testing.T) {
ver := version
if err := test.in.ConvertTo(context.Background(), ver); err != nil {
if !test.wantErr {
t.Errorf("ConvertTo() = %v", err)
}
return
}
t.Logf("ConvertTo() = %#v", ver)
got := &Pipeline{}
if err := got.ConvertFrom(context.Background(), ver); err != nil {
t.Errorf("ConvertFrom() = %v", err)
}
t.Logf("ConvertFrom() = %#v", got)
if d := cmp.Diff(test.in, got); d != "" {
t.Errorf("roundtrip %s", diff.PrintWantGot(d))
if err := test.in.ConvertTo(context.Background(), ver); err == nil {
t.Errorf("Expected ConvertTo to fail but did not produce any error")
}
return
})
}
}
}

func TestPipelineConversionFromWithFinally(t *testing.T) {
versions := []apis.Convertible{&v1beta1.Pipeline{}}
p := &Pipeline{
ObjectMeta: metav1.ObjectMeta{
Name: "foo",
Namespace: "bar",
Generation: 1,
},
Spec: PipelineSpec{
Tasks: []PipelineTask{{Name: "mytask", TaskRef: &TaskRef{Name: "task"}}},
},
}
for _, version := range versions {
Copy link
Collaborator

Choose a reason for hiding this comment

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

im a bit confused about why there is a loop here when it seems like we only have one thing in versions?

Copy link
Member Author

Choose a reason for hiding this comment

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

yup its iterating over a list of versions which has only one version today, being a list, it would be easier to add new versions in future if we had to, v1beta2 or v2beta1 but we might not have v1alpha1 at that point, no strong preference either ways ...

Copy link
Collaborator

Choose a reason for hiding this comment

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

personally id still prefer to avoid the loop when we don't need it (YAGNI!) but if that's the only piece of feedback I have then lets just go with it as is :D

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to skip the loop to for now.
In L220 we cast to (*v1beta1.Pipeline), which would not work with other versions.
I think we would need to introduce a WithFinally interface to have this generic loop.

t.Run("finally not available in v1alpha1", func(t *testing.T) {
ver := version
// convert v1alpha1 to v1beta1
if err := p.ConvertTo(context.Background(), ver); err != nil {
t.Errorf("ConvertTo() = %v", err)
}
// modify ver to introduce new field which causes failure to convert v1beta1 to v1alpha1
source := ver
source.(*v1beta1.Pipeline).Spec.Finally = []v1beta1.PipelineTask{{Name: "finaltask", TaskRef: &TaskRef{Name: "task"}}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

i feel like it might be worth including an end to end test for this, e.g. one that creates a v1alpha1 object with this field and then gets an error. since we're manipulating a v1beta1 object here its a bit hard to feel completely certain that this is an accurate representation of what would happen to a user

Copy link
Member Author

Choose a reason for hiding this comment

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

Added one test here to convert v1beta1 object to v1alpha1 which results in failure since original v1beta1 contains finally

got := &Pipeline{}
if err := got.ConvertFrom(context.Background(), source); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

when would the v1alpha1 ConvertFrom get called and why? I would guess we are always converting from v1alpha1 to v1beta1 and never the other way?

Copy link
Member Author

Choose a reason for hiding this comment

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

ConvertTo converts v1alpha1 to the specified version which is v1beta1 here, ConvertFrom converts v1beta1 back to v1alpha1, got is initialized with empty v1alpha1 Pipeline, ConvertFrom converts source which is v1beta1 in this case to v1alpha1 and stores the converted Pipeline in got.

Copy link
Collaborator

Choose a reason for hiding this comment

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

im still a bit confused but ill go with it! i just dont understand why we go back to v1alpha1 (via ConvertFrom) at all - maybe that's after the controller is done reconciling, before we store the state back into etcd?

Copy link
Member

Choose a reason for hiding this comment

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

See my question above to @vdemeester and his reply.
As long as we serve v1alpha1 too, tools may request it. Since our pipelines are stored in v1beta1, to serve a v1alpha1 request we need to covert back to v1alpha1... which I feel it's like a slightly risky thing to do, but probably still better than not serving v1alpha1 anymore at all.

cce, ok := err.(*CannotConvertError)
// conversion error contains the field name which resulted in the failure and should be equal to "Finally" here
if ok && cce.Field == FinallyFieldName {
return
}
t.Errorf("ConvertFrom() should have failed")
}
})
}
}

func TestPipelineConversionFromBetaToAlphaWithFinally_Failure(t *testing.T) {
p := &v1beta1.Pipeline{
ObjectMeta: metav1.ObjectMeta{
Name: "foo",
Namespace: "bar",
Generation: 1,
},
Spec: v1beta1.PipelineSpec{
Tasks: []v1beta1.PipelineTask{{Name: "mytask", TaskRef: &TaskRef{Name: "task"}}},
Finally: []v1beta1.PipelineTask{{Name: "mytask", TaskRef: &TaskRef{Name: "task"}}},
},
}
t.Run("finally not available in v1alpha1", func(t *testing.T) {
got := &Pipeline{}
if err := got.ConvertFrom(context.Background(), p); err != nil {
cce, ok := err.(*CannotConvertError)
// conversion error (cce) contains the field name which resulted in the failure and should be equal to "finally" here
if ok && cce.Field == FinallyFieldName {
return
}
t.Errorf("ConvertFrom() should have failed")
}
})
}
10 changes: 10 additions & 0 deletions pkg/apis/pipeline/v1beta1/pipeline_defaults.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,4 +42,14 @@ func (ps *PipelineSpec) SetDefaults(ctx context.Context) {
for i := range ps.Params {
ps.Params[i].SetDefaults(ctx)
}
for _, ft := range ps.Finally {
if ft.TaskRef != nil {
if ft.TaskRef.Kind == "" {
ft.TaskRef.Kind = NamespacedTaskKind
}
}
if ft.TaskSpec != nil {
ft.TaskSpec.SetDefaults(ctx)
}
}
}
136 changes: 136 additions & 0 deletions pkg/apis/pipeline/v1beta1/pipeline_defaults_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,136 @@
/*
Copyright 2020 The Tekton Authors

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package v1beta1_test

import (
"context"
"testing"

"github.com/tektoncd/pipeline/test/diff"

"github.com/google/go-cmp/cmp"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
)

func TestPipeline_SetDefaults(t *testing.T) {
p := &v1beta1.Pipeline{}
want := &v1beta1.Pipeline{}
ctx := context.Background()
p.SetDefaults(ctx)
if d := cmp.Diff(want, p); d != "" {
t.Errorf("Mismatch of Pipeline: empty pipeline must not change after setting defaults: %s", diff.PrintWantGot(d))
}
}

func TestPipelineSpec_SetDefaults(t *testing.T) {
cases := []struct {
desc string
ps *v1beta1.PipelineSpec
want *v1beta1.PipelineSpec
}{{
desc: "empty pipelineSpec must not change after setting defaults",
ps: &v1beta1.PipelineSpec{},
want: &v1beta1.PipelineSpec{},
}, {
desc: "pipeline task - default task kind must be " + string(v1beta1.NamespacedTaskKind),
ps: &v1beta1.PipelineSpec{
Tasks: []v1beta1.PipelineTask{{
Name: "foo", TaskRef: &v1beta1.TaskRef{Name: "foo-task"},
}},
},
want: &v1beta1.PipelineSpec{
Tasks: []v1beta1.PipelineTask{{
Name: "foo", TaskRef: &v1beta1.TaskRef{Name: "foo-task", Kind: v1beta1.NamespacedTaskKind},
}},
},
}, {
desc: "final pipeline task - default task kind must be " + string(v1beta1.NamespacedTaskKind),
ps: &v1beta1.PipelineSpec{
Finally: []v1beta1.PipelineTask{{
Name: "final-task", TaskRef: &v1beta1.TaskRef{Name: "foo-task"},
}},
},
want: &v1beta1.PipelineSpec{
Finally: []v1beta1.PipelineTask{{
Name: "final-task", TaskRef: &v1beta1.TaskRef{Name: "foo-task", Kind: v1beta1.NamespacedTaskKind},
}},
},
}, {
desc: "param type - default param type must be " + string(v1beta1.ParamTypeString),
ps: &v1beta1.PipelineSpec{
Params: []v1beta1.ParamSpec{{
Name: "string-param",
}},
},
want: &v1beta1.PipelineSpec{
Params: []v1beta1.ParamSpec{{
Name: "string-param", Type: v1beta1.ParamTypeString,
}},
},
}, {
desc: "pipeline task with taskSpec - default param type must be " + string(v1beta1.ParamTypeString),
ps: &v1beta1.PipelineSpec{
Tasks: []v1beta1.PipelineTask{{
Name: "foo", TaskSpec: &v1beta1.TaskSpec{
Params: []v1beta1.ParamSpec{{
Name: "string-param",
}},
},
}},
},
want: &v1beta1.PipelineSpec{
Tasks: []v1beta1.PipelineTask{{
Name: "foo", TaskSpec: &v1beta1.TaskSpec{
Params: []v1beta1.ParamSpec{{
Name: "string-param",
Type: v1beta1.ParamTypeString,
}},
},
}},
},
}, {
desc: "final pipeline task with taskSpec - default param type must be " + string(v1beta1.ParamTypeString),
ps: &v1beta1.PipelineSpec{
Finally: []v1beta1.PipelineTask{{
Name: "foo", TaskSpec: &v1beta1.TaskSpec{
Params: []v1beta1.ParamSpec{{
Name: "string-param",
}},
},
}},
},
want: &v1beta1.PipelineSpec{
Finally: []v1beta1.PipelineTask{{
Name: "foo", TaskSpec: &v1beta1.TaskSpec{
Params: []v1beta1.ParamSpec{{
Name: "string-param",
Type: v1beta1.ParamTypeString,
}},
},
}},
},
}}
for _, tc := range cases {
t.Run(tc.desc, func(t *testing.T) {
ctx := context.Background()
tc.ps.SetDefaults(ctx)
if d := cmp.Diff(tc.want, tc.ps); d != "" {
t.Errorf("Mismatch of pipelineSpec after setting defaults: %s: %s", tc.desc, diff.PrintWantGot(d))
}
})
}
}
pritidesai marked this conversation as resolved.
Show resolved Hide resolved
4 changes: 4 additions & 0 deletions pkg/apis/pipeline/v1beta1/pipeline_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,10 @@ type PipelineSpec struct {
// Results are values that this pipeline can output once run
// +optional
Results []PipelineResult `json:"results,omitempty"`
// Finally declares the list of Tasks that execute just before leaving the Pipeline
// i.e. either after all Tasks are finished executing successfully
// or after a failure which would result in ending the Pipeline
Finally []PipelineTask `json:"finally,omitempty"`
}

// PipelineResult used to describe the results of a pipeline
Expand Down
Loading