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

Serialize entire v1beta1 PipelineSpec when converting to v1alpha #3854

Closed
wants to merge 1 commit into from
Closed

Serialize entire v1beta1 PipelineSpec when converting to v1alpha #3854

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
101 changes: 74 additions & 27 deletions pkg/apis/pipeline/v1alpha1/pipeline_conversion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,25 +27,39 @@ import (
"knative.dev/pkg/apis"
)

const FinallyFieldName = "finally"
// This annotation was used to serialize and deserialize Finally tasks in the
// 0.22 Pipelines release. Keep it around to continue supporting any documents
// converted during that time.
Copy link
Collaborator

Choose a reason for hiding this comment

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

would it make sense to add this to our doc tracking deprecations?

const finallyAnnotationKey = "tekton.dev/v1beta1Finally"

const V1Beta1PipelineSpecSerializedAnnotationKey = "tekton.dev/v1beta1PipelineSpec"

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

// ConvertTo implements api.Convertible
func (source *Pipeline) ConvertTo(ctx context.Context, obj apis.Convertible) error {
switch sink := obj.(type) {
case *v1beta1.Pipeline:
sink.ObjectMeta = source.ObjectMeta
if err := source.Spec.ConvertTo(ctx, &sink.Spec); err != nil {
if hasV1Beta1PipelineSpecAnnotation(sink.ObjectMeta.Annotations) {
if err := deserializeIntoV1Beta1PipelineSpec(&sink.ObjectMeta, &sink.Spec); err != nil {
return fmt.Errorf("error deserializing v1beta1 annotation into spec: %w", err)
}
} else if err := source.Spec.ConvertTo(ctx, &sink.Spec); err != nil {
return err
}
if err := deserializeFinally(&sink.ObjectMeta, &sink.Spec); err != nil {
return err
}
if err := v1beta1.ValidatePipelineTasks(ctx, sink.Spec.Tasks, sink.Spec.Finally); err != nil {
return fmt.Errorf("error converting finally annotation into beta field: %w", err)
}

// The defaults and validation criteria might have changed since this
// resource was serialized. This would likely be the result of a bug
// in Pipelines so this is intended to help prevent or surface those
// potential bugs.
sink.SetDefaults(ctx)
if err := sink.Validate(ctx); err != nil {
return fmt.Errorf("invalid v1beta1 pipeline after conversion: %w", err)
}
default:
return fmt.Errorf("unknown version, got: %T", sink)
}
Expand All @@ -57,6 +71,7 @@ func (source *PipelineSpec) ConvertTo(ctx context.Context, sink *v1beta1.Pipelin
sink.Params = source.Params
sink.Workspaces = source.Workspaces
sink.Description = source.Description
sink.Results = source.Results
if len(source.Tasks) > 0 {
sink.Tasks = make([]v1beta1.PipelineTask, len(source.Tasks))
for i := range source.Tasks {
Expand Down Expand Up @@ -93,7 +108,7 @@ func (sink *Pipeline) ConvertFrom(ctx context.Context, obj apis.Convertible) err
switch source := obj.(type) {
case *v1beta1.Pipeline:
sink.ObjectMeta = source.ObjectMeta
if err := serializeFinally(&sink.ObjectMeta, source.Spec.Finally); err != nil {
if err := serializeV1Beta1PipelineSpec(&sink.ObjectMeta, &source.Spec); err != nil {
return err
}
return sink.Spec.ConvertFrom(ctx, source.Spec)
Expand All @@ -107,6 +122,7 @@ func (sink *PipelineSpec) ConvertFrom(ctx context.Context, source v1beta1.Pipeli
sink.Params = source.Params
sink.Workspaces = source.Workspaces
sink.Description = source.Description
sink.Results = source.Results
Copy link
Collaborator

Choose a reason for hiding this comment

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

im curious why this was missing before?

Copy link
Author

@ghost ghost Apr 2, 2021 •
edited by ghost
Loading

Choose a reason for hiding this comment

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

We didn't preserve Pipeline Results on the roundtrip to v1alpha1 and back :S so users were potentially losing this part of their pipeline specs prior to this commit. 🐛

if len(source.Tasks) > 0 {
sink.Tasks = make([]PipelineTask, len(source.Tasks))
for i := range source.Tasks {
Expand Down Expand Up @@ -137,39 +153,70 @@ func (sink *PipelineTask) ConvertFrom(ctx context.Context, source v1beta1.Pipeli
return nil
}

// serializeFinally serializes a list of Finally Tasks to the annotations
// of an object's metadata section. This can then be used to re-instantiate
// the Finally Tasks when converting back up to v1beta1 and beyond.
func serializeFinally(meta *metav1.ObjectMeta, finally []v1beta1.PipelineTask) error {
if len(finally) != 0 {
b, err := json.Marshal(finally)
if err != nil {
return err
}
if meta.Annotations == nil {
meta.Annotations = make(map[string]string)
}
meta.Annotations[finallyAnnotationKey] = string(b)
}
return nil
}

// deserializeFinally populates a PipelineSpec's Finally list
// from an annotation found on resources that have been previously
// converted down from v1beta1 to v1alpha1.
// from an annotation found on resources that were converted
// from v1beta1 to v1alpha1. This annotation was only used
// for a short time (the 0.22 release) but is kept here to
// continue supporting any documents converted to v1alpha1 during
// that period.
func deserializeFinally(meta *metav1.ObjectMeta, spec *v1beta1.PipelineSpec) error {
if meta.Annotations != nil {
if str, ok := meta.Annotations[finallyAnnotationKey]; ok {
finally := []v1beta1.PipelineTask{}
if err := json.Unmarshal([]byte(str), &finally); err != nil {
return fmt.Errorf("error converting finally annotation into beta field: %w", err)
}
delete(meta.Annotations, finallyAnnotationKey)
if len(meta.Annotations) == 0 {
if len(meta.Annotations) == 1 {
meta.Annotations = nil
} else {
delete(meta.Annotations, finallyAnnotationKey)
}
spec.Finally = finally
}
}
return nil
}

// hasV1Beta1PipelineSpecAnnotation returns true if the provided annotations
// map contains the key expected for serialized v1beta1 PipelineSpecs.
func hasV1Beta1PipelineSpecAnnotation(annotations map[string]string) bool {
if annotations == nil {
return false
}
_, has := annotations[V1Beta1PipelineSpecSerializedAnnotationKey]
return has
}

// serializeV1Beta1PipelineSpec serializes a v1beta1.PipelineSpec to an
// annotation in the provided meta. This will be used when applying the
// v1alpha1 resource back to the cluster in order to rehydrate a v1beta1 doc
// exactly as it appeared before being converted.
func serializeV1Beta1PipelineSpec(meta *metav1.ObjectMeta, spec *v1beta1.PipelineSpec) error {
b, err := json.Marshal(spec)
if err != nil {
return fmt.Errorf("error serializing v1beta1 document into annotation: %w", err)
}
if meta.Annotations == nil {
meta.Annotations = make(map[string]string)
}
meta.Annotations[V1Beta1PipelineSpecSerializedAnnotationKey] = string(b)
return nil
}

// deserializeIntoV1Beta1PipelineSpec populates a PipelineSpec
// from a JSON annotation found on resources that have been previously
// converted from v1beta1 to v1alpha1.
func deserializeIntoV1Beta1PipelineSpec(meta *metav1.ObjectMeta, spec *v1beta1.PipelineSpec) error {
if meta.Annotations != nil {
if str, ok := meta.Annotations[V1Beta1PipelineSpecSerializedAnnotationKey]; ok {
if err := json.Unmarshal([]byte(str), spec); err != nil {
return fmt.Errorf("error deserializing v1beta1 document from annotation: %w", err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe im overthinking this but im wondering about the impact of this annotation becoming malformed; i.e. do we want to have an error or just ignore it? (i guess having an error is safer in the long run?)

}
delete(meta.Annotations, V1Beta1PipelineSpecSerializedAnnotationKey)
if len(meta.Annotations) == 0 {
meta.Annotations = nil
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

im curious why we are explicitly setting this to nil? (maybe a comment would help)

}
}
return nil
}
160 changes: 133 additions & 27 deletions pkg/apis/pipeline/v1alpha1/pipeline_conversion_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,32 @@ import (
"github.com/tektoncd/pipeline/test/diff"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/selection"
"knative.dev/pkg/apis"
)

var ignoreAnnotations = cmp.FilterPath(func(p cmp.Path) bool {
if len(p) < 2 {
return false
}
structField, ok := p.Index(-2).(cmp.StructField)
if !ok {
return false
}
if structField.Name() != "ObjectMeta" {
return false
}

structField, ok = p.Index(-1).(cmp.StructField)
Copy link
Collaborator

Choose a reason for hiding this comment

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

the -1 and -2 to me feel like they could use a comment or variable to add some more context

if !ok {
return false
}
if structField.Name() != "Annotations" {
return false
}
return true
}, cmp.Ignore())

func TestPipelineConversionBadType(t *testing.T) {
good, bad := &Pipeline{}, &Task{}

Expand Down Expand Up @@ -81,8 +104,7 @@ func TestPipelineConversion_Success(t *testing.T) {
Conditions: []PipelineTaskCondition{{
ConditionRef: "condition1",
}},
Retries: 10,
RunAfter: []string{"task1"},
Retries: 10,
Resources: &PipelineTaskResources{
Inputs: []v1beta1.PipelineTaskInputResource{{
Name: "input1",
Expand Down Expand Up @@ -128,9 +150,8 @@ func TestPipelineConversion_Success(t *testing.T) {
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 != "" {

if d := cmp.Diff(test.in, got, ignoreAnnotations); d != "" {
t.Errorf("roundtrip %s", diff.PrintWantGot(d))
}
})
Expand Down Expand Up @@ -219,40 +240,125 @@ func TestPipelineConversionFromWithFinally(t *testing.T) {
source.(*v1beta1.Pipeline).Spec.Finally = []v1beta1.PipelineTask{{Name: "finaltask", TaskRef: &TaskRef{Name: "task"}}}
got := &Pipeline{}
if err := got.ConvertFrom(context.Background(), source); err != nil {
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")
t.Errorf("ConvertFrom() unexpected error: %v", err)
}
})
}
}

func TestPipelineConversionFromBetaToAlphaWithFinally(t *testing.T) {
// TestV1Beta1PipelineConversionRoundTrip checks that a populated v1beta1 pipeline
// correctly round-trips through v1alpha1 conversion and back again without losing
// any v1beta1-specific fields.
func TestV1Beta1PipelineConversionRoundTrip(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: "myfinallytask", TaskRef: &TaskRef{Name: "task"}}},
Tasks: []v1beta1.PipelineTask{
{Name: "mytask", TaskRef: &TaskRef{Kind: "Task", Name: "task"}},
{
Name: "task-with-when",
TaskRef: &TaskRef{Kind: "Task", Name: "task"},
WhenExpressions: v1beta1.WhenExpressions{{
Input: "test",
Operator: selection.NotIn,
Values: []string{"foo", "bar"},
}},
},
},
Finally: []v1beta1.PipelineTask{{Name: "myfinallytask", TaskRef: &TaskRef{Kind: "Task", Name: "task"}}},
},
}
t.Run("finally stored by v1alpha1 and rehydrated for v1beta1", func(t *testing.T) {
downgrade := &Pipeline{}
if err := downgrade.ConvertFrom(context.Background(), p); err != nil {
t.Errorf("error converting from v1beta1 with finally field to v1alpha1 with finally annotation: %v", err)
}
upgrade := &v1beta1.Pipeline{}
if err := downgrade.ConvertTo(context.Background(), upgrade); err != nil {
t.Errorf("error converting from v1alpha1 with finally annotation to v1beta1 with finally field: %v", err)
}
if d := cmp.Diff(p, upgrade); d != "" {
t.Errorf("unexpected difference between v1beta1 with finally field and round-tripped v1beta1 with finally field: %s", diff.PrintWantGot(d))
}
})
downgrade := &Pipeline{}
if err := downgrade.ConvertFrom(context.Background(), p); err != nil {
t.Errorf("error converting from v1beta1 to v1alpha1: %v", err)
}
upgrade := &v1beta1.Pipeline{}
if err := downgrade.ConvertTo(context.Background(), upgrade); err != nil {
t.Errorf("error converting from v1alpha1 to v1beta1: %v", err)
}
if d := cmp.Diff(p, upgrade); d != "" {
t.Errorf("unexpected difference between original and round-tripped v1beta1 document: %s", diff.PrintWantGot(d))
}
}

// TestConvertToDeserializationErrors confirms that errors are returned
// for invalid serialized data.
func TestConvertToDeserializationErrors(t *testing.T) {
for _, tc := range []struct {
name string
source Pipeline
}{
{
name: "invalid v1beta1 spec annotation",
source: Pipeline{
ObjectMeta: metav1.ObjectMeta{
Annotations: map[string]string{
V1Beta1PipelineSpecSerializedAnnotationKey: "invalid json",
},
},
},
},
{
name: "invalid finally annotation",
source: Pipeline{
ObjectMeta: metav1.ObjectMeta{
Annotations: map[string]string{
finallyAnnotationKey: "invalid json",
},
},
},
},
} {
t.Run(tc.name, func(t *testing.T) {
sink := &v1beta1.Pipeline{}
err := tc.source.ConvertTo(context.Background(), sink)
t.Logf("received error %q", err)
if err == nil {
t.Errorf("expected error but received none")
}
})
}
}

// TestAlphaPipelineConversionWithFinallyAnnotation tests that a v1alpha1
// pipeline with a specific annotation correctly gets turned into a v1beta1
// pipeline with Finally section. The annotation was given to pipelines with
// Finally when converted from v1beta1 to v1alpha1 in the 0.22 Pipelines
// release.
func TestAlphaPipelineConversionWithFinallyAnnotation(t *testing.T) {
p := &Pipeline{
ObjectMeta: metav1.ObjectMeta{
Name: "foo",
Namespace: "bar",
Generation: 1,
Annotations: map[string]string{
finallyAnnotationKey: `[{"name": "myfinallytask", "taskRef": {"name": "task"}}]`,
},
},
Spec: PipelineSpec{
Tasks: []PipelineTask{{Name: "task1", TaskRef: &TaskRef{Name: "task1"}}},
},
}
expected := &v1beta1.Pipeline{
ObjectMeta: metav1.ObjectMeta{
Name: "foo",
Namespace: "bar",
Generation: 1,
},
Spec: v1beta1.PipelineSpec{
Tasks: []v1beta1.PipelineTask{{Name: "task1", TaskRef: &TaskRef{Name: "task1", Kind: "Task"}}},
Finally: []v1beta1.PipelineTask{{Name: "myfinallytask", TaskRef: &TaskRef{Kind: "Task", Name: "task"}}},
},
}
upgrade := &v1beta1.Pipeline{}
if err := p.ConvertTo(context.Background(), upgrade); err != nil {
t.Errorf("error converting from v1alpha1 to v1beta1: %v", err)
}
if d := cmp.Diff(expected, upgrade); d != "" {
t.Errorf("unexpected difference: %s", diff.PrintWantGot(d))
}
}
Loading