diff --git a/pkg/builder/cluster/builder.go b/pkg/builder/cluster/builder.go index 32c9ea5d..34f7de69 100644 --- a/pkg/builder/cluster/builder.go +++ b/pkg/builder/cluster/builder.go @@ -46,7 +46,7 @@ func (op *operation) Name() string { return op.name } -func (op *operation) Checkpoint(status *v1alpha1.BuildStatus) error { +func (op *operation) Checkpoint(build *v1alpha1.Build, status *v1alpha1.BuildStatus) error { status.Builder = v1alpha1.ClusterBuildProvider if status.Cluster == nil { status.Cluster = &v1alpha1.ClusterSpec{} @@ -57,7 +57,19 @@ func (op *operation) Checkpoint(status *v1alpha1.BuildStatus) error { status.StartTime = op.builder.podCreationTime status.StepStates = nil status.StepsCompleted = nil - for _, s := range op.statuses { + + // Always ignore the first pod status, which is creds-init. + skip := 1 + if build.Spec.Source != nil { + // If the build specifies source, skip another container status, which + // is the source-fetching container. + skip++ + } + if skip > len(op.statuses) { + skip = 0 + } + + for _, s := range op.statuses[skip:] { if s.State.Terminated != nil { status.StepsCompleted = append(status.StepsCompleted, s.Name) } diff --git a/pkg/builder/cluster/builder_test.go b/pkg/builder/cluster/builder_test.go index cd48e0c0..9e7cd5b4 100644 --- a/pkg/builder/cluster/builder_test.go +++ b/pkg/builder/cluster/builder_test.go @@ -60,23 +60,25 @@ func TestBasicFlow(t *testing.T) { t.Fatalf("Unexpected error executing builder.Build: %v", err) } - var bs v1alpha1.BuildStatus - if err := op.Checkpoint(&bs); err != nil { + build := &v1alpha1.Build{ + Status: v1alpha1.BuildStatus{}, + } + if err := op.Checkpoint(build, &build.Status); err != nil { t.Fatalf("Unexpected error executing op.Checkpoint: %v", err) } - if buildercommon.IsDone(&bs) { - t.Errorf("IsDone(%v); wanted not done, got done.", bs) + if buildercommon.IsDone(&build.Status) { + t.Errorf("IsDone(%v); wanted not done, got done.", build.Status) } - if bs.CreationTime.IsZero() { - t.Errorf("bs.CreationTime; want zero, got %v", bs.CreationTime) + if build.Status.CreationTime.IsZero() { + t.Errorf("build.Status.CreationTime; want zero, got %v", build.Status.CreationTime) } - if !bs.CompletionTime.IsZero() { - t.Errorf("bs.CompletionTime; want zero, got %v", bs.CompletionTime) + if !build.Status.CompletionTime.IsZero() { + t.Errorf("build.Status.CompletionTime; want zero, got %v", build.Status.CompletionTime) } - if !bs.StartTime.IsZero() { - t.Errorf("bs.StartTime; want non-zero, got %v", bs.StartTime) + if !build.Status.StartTime.IsZero() { + t.Errorf("build.Status.StartTime; want non-zero, got %v", build.Status.StartTime) } - op, err = builder.OperationFromStatus(&bs) + op, err = builder.OperationFromStatus(&build.Status) if err != nil { t.Fatalf("Unexpected error executing OperationFromStatus: %v", err) } @@ -163,23 +165,25 @@ func TestNonFinalUpdateFlow(t *testing.T) { t.Fatalf("Unexpected error executing builder.Build: %v", err) } - var bs v1alpha1.BuildStatus - if err := op.Checkpoint(&bs); err != nil { + build := &v1alpha1.Build{ + Status: v1alpha1.BuildStatus{}, + } + if err := op.Checkpoint(build, &build.Status); err != nil { t.Fatalf("Unexpected error executing op.Checkpoint: %v", err) } - if buildercommon.IsDone(&bs) { - t.Errorf("IsDone(%v); wanted not done, got done.", bs) + if buildercommon.IsDone(&build.Status) { + t.Errorf("IsDone(%v); wanted not done, got done.", build.Status) } - if bs.CreationTime.IsZero() { - t.Errorf("bs.CreationTime; want zero, got %v", bs.CreationTime) + if build.Status.CreationTime.IsZero() { + t.Errorf("build.Status.CreationTime; want zero, got %v", build.Status.CreationTime) } - if !bs.CompletionTime.IsZero() { - t.Errorf("bs.CompletionTime; want zero, got %v", bs.CompletionTime) + if !build.Status.CompletionTime.IsZero() { + t.Errorf("build.Status.CompletionTime; want zero, got %v", build.Status.CompletionTime) } - if !bs.StartTime.IsZero() { - t.Errorf("bs.StartTime; want non-zero, got %v", bs.StartTime) + if !build.Status.StartTime.IsZero() { + t.Errorf("build.Status.StartTime; want non-zero, got %v", build.Status.StartTime) } - op, err = builder.OperationFromStatus(&bs) + op, err = builder.OperationFromStatus(&build.Status) if err != nil { t.Fatalf("Unexpected error executing OperationFromStatus: %v", err) } @@ -260,23 +264,25 @@ func TestFailureFlow(t *testing.T) { t.Fatalf("Unexpected error executing builder.Build: %v", err) } - var bs v1alpha1.BuildStatus - if err := op.Checkpoint(&bs); err != nil { + build := &v1alpha1.Build{ + Status: v1alpha1.BuildStatus{}, + } + if err := op.Checkpoint(build, &build.Status); err != nil { t.Fatalf("Unexpected error executing op.Checkpoint: %v", err) } - if buildercommon.IsDone(&bs) { - t.Errorf("IsDone(%v); wanted not done, got done.", bs) + if buildercommon.IsDone(&build.Status) { + t.Errorf("IsDone(%v); wanted not done, got done.", build.Status) } - if bs.CreationTime.IsZero() { - t.Errorf("bs.CreationTime; want zero, got %v", bs.CreationTime) + if build.Status.CreationTime.IsZero() { + t.Errorf("build.Status.CreationTime; want zero, got %v", build.Status.CreationTime) } - if !bs.CompletionTime.IsZero() { - t.Errorf("bs.CompletionTime; want zero, got %v", bs.CompletionTime) + if !build.Status.CompletionTime.IsZero() { + t.Errorf("build.Status.CompletionTime; want zero, got %v", build.Status.CompletionTime) } - if !bs.StartTime.IsZero() { - t.Errorf("bs.StartTime; want non-zero, got %v", bs.StartTime) + if !build.Status.StartTime.IsZero() { + t.Errorf("build.Status.StartTime; want non-zero, got %v", build.Status.StartTime) } - op, err = builder.OperationFromStatus(&bs) + op, err = builder.OperationFromStatus(&build.Status) if err != nil { t.Fatalf("Unexpected error executing OperationFromStatus: %v", err) } @@ -365,23 +371,25 @@ func TestPodPendingFlow(t *testing.T) { t.Fatalf("Unexpected error executing builder.Build: %v", err) } - var bs v1alpha1.BuildStatus - if err := op.Checkpoint(&bs); err != nil { + build := &v1alpha1.Build{ + Status: v1alpha1.BuildStatus{}, + } + if err := op.Checkpoint(build, &build.Status); err != nil { t.Fatalf("Unexpected error executing op.Checkpoint: %v", err) } - if buildercommon.IsDone(&bs) { - t.Errorf("IsDone(%v); wanted not done, got done.", bs) + if buildercommon.IsDone(&build.Status) { + t.Errorf("IsDone(%v); wanted not done, got done.", build.Status) } - if bs.CreationTime.IsZero() { - t.Errorf("bs.CreationTime; want zero, got %v", bs.CreationTime) + if build.Status.CreationTime.IsZero() { + t.Errorf("build.Status.CreationTime; want zero, got %v", build.Status.CreationTime) } - if !bs.CompletionTime.IsZero() { - t.Errorf("bs.CompletionTime; want zero, got %v", bs.CompletionTime) + if !build.Status.CompletionTime.IsZero() { + t.Errorf("build.Status.CompletionTime; want zero, got %v", build.Status.CompletionTime) } - if !bs.StartTime.IsZero() { - t.Errorf("bs.StartTime; want non-zero, got %v", bs.StartTime) + if !build.Status.StartTime.IsZero() { + t.Errorf("build.Status.StartTime; want non-zero, got %v", build.Status.StartTime) } - op, err = builder.OperationFromStatus(&bs) + op, err = builder.OperationFromStatus(&build.Status) if err != nil { t.Fatalf("Unexpected error executing OperationFromStatus: %v", err) } @@ -479,23 +487,25 @@ func TestStepFailureFlow(t *testing.T) { t.Fatalf("Unexpected error executing builder.Build: %v", err) } - var bs v1alpha1.BuildStatus - if err := op.Checkpoint(&bs); err != nil { + build := &v1alpha1.Build{ + Status: v1alpha1.BuildStatus{}, + } + if err := op.Checkpoint(build, &build.Status); err != nil { t.Fatalf("Unexpected error executing op.Checkpoint: %v", err) } - if buildercommon.IsDone(&bs) { - t.Errorf("IsDone(%v); wanted not done, got done.", bs) + if buildercommon.IsDone(&build.Status) { + t.Errorf("IsDone(%v); wanted not done, got done.", build.Status) } - if bs.CreationTime.IsZero() { - t.Errorf("bs.CreationTime; want zero, got %v", bs.CreationTime) + if build.Status.CreationTime.IsZero() { + t.Errorf("build.Status.CreationTime; want zero, got %v", build.Status.CreationTime) } - if !bs.StartTime.IsZero() { - t.Errorf("bs.StartTime; want non-zero, got %v", bs.StartTime) + if !build.Status.StartTime.IsZero() { + t.Errorf("build.Status.StartTime; want non-zero, got %v", build.Status.StartTime) } - if !bs.CompletionTime.IsZero() { - t.Errorf("bs.CompletionTime; want zero, got %v", bs.CompletionTime) + if !build.Status.CompletionTime.IsZero() { + t.Errorf("build.Status.CompletionTime; want zero, got %v", build.Status.CompletionTime) } - op, err = builder.OperationFromStatus(&bs) + op, err = builder.OperationFromStatus(&build.Status) if err != nil { t.Fatalf("Unexpected error executing OperationFromStatus: %v", err) } @@ -642,3 +652,77 @@ func statusMessage(status *v1alpha1.BuildStatus) string { } return "" } + +func TestStripStepStates(t *testing.T) { + for _, c := range []struct { + desc string + statuses []corev1.ContainerStatus + build *v1alpha1.Build + }{{ + desc: "only creds-init", + statuses: []corev1.ContainerStatus{{ + State: corev1.ContainerState{Terminated: &corev1.ContainerStateTerminated{Reason: "creds-init: should be stripped"}}, + }, { + State: corev1.ContainerState{Terminated: &corev1.ContainerStateTerminated{Reason: "real step: should be retained"}}, + }}, + build: &v1alpha1.Build{ + // No source. + Status: v1alpha1.BuildStatus{}, + }, + }, { + desc: "has source", + statuses: []corev1.ContainerStatus{{ + Name: "creds-init", + State: corev1.ContainerState{Terminated: &corev1.ContainerStateTerminated{Reason: "creds-init: should be stripped"}}, + }, { + Name: "git-init", + State: corev1.ContainerState{Terminated: &corev1.ContainerStateTerminated{Reason: "git-init: should be stripped"}}, + }, { + State: corev1.ContainerState{Terminated: &corev1.ContainerStateTerminated{Reason: "real step: should be retained"}}, + }}, + build: &v1alpha1.Build{ + // No source. + Spec: v1alpha1.BuildSpec{ + Source: &v1alpha1.SourceSpec{ + Git: &v1alpha1.GitSourceSpec{}, + }, + }, + Status: v1alpha1.BuildStatus{}, + }, + }} { + t.Run(c.desc, func(t *testing.T) { + status := &v1alpha1.BuildStatus{} + op := &operation{ + statuses: c.statuses, + builder: &builder{}, + namespace: "namespace", + name: "podname", + } + if err := op.Checkpoint(c.build, status); err != nil { + t.Fatalf("Checkpoint: %v", err) + } + + // In both cases, we want the same status, stripped of implicit + // steps' states. + wantStatus := &v1alpha1.BuildStatus{ + Builder: v1alpha1.ClusterBuildProvider, + Cluster: &v1alpha1.ClusterSpec{ + Namespace: "namespace", + PodName: "podname", + }, + StepStates: []corev1.ContainerState{{ + Terminated: &corev1.ContainerStateTerminated{Reason: "real step: should be retained"}, + }}, + StepsCompleted: []string{""}, + Conditions: []v1alpha1.BuildCondition{{ + Type: v1alpha1.BuildSucceeded, + Status: corev1.ConditionUnknown, + Reason: "Building", + }}, + } + if d := buildtest.JSONDiff(wantStatus, status); d != "" { + t.Errorf("Diff:\n%s", d) + } + }) + } +} diff --git a/pkg/builder/cluster/convert/convert_test.go b/pkg/builder/cluster/convert/convert_test.go index 83752889..866b447a 100644 --- a/pkg/builder/cluster/convert/convert_test.go +++ b/pkg/builder/cluster/convert/convert_test.go @@ -17,10 +17,8 @@ limitations under the License. package convert import ( - "encoding/json" "testing" - "github.com/sergi/go-diff/diffmatchpatch" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" fakek8s "k8s.io/client-go/kubernetes/fake" @@ -123,7 +121,7 @@ func TestRoundtrip(t *testing.T) { t.Fatalf("Unable to convert %q to CRD: %v", in, err) } - if d := diff(og, b); d != "" { + if d := buildtest.JSONDiff(og, b); d != "" { t.Errorf("Diff:\n%s", d) } }) @@ -348,29 +346,9 @@ func TestFromCRD(t *testing.T) { t.Fatalf("FromCRD: %v", err) } - if d := diff(got.Spec, c.want); d != "" { + if d := buildtest.JSONDiff(got.Spec, c.want); d != "" { t.Errorf("Diff:\n%s", d) } }) } } - -func diff(l, r interface{}) string { - lb, err := json.MarshalIndent(l, "", " ") - if err != nil { - panic(err.Error()) - } - rb, err := json.MarshalIndent(r, "", " ") - if err != nil { - panic(err.Error()) - } - - dmp := diffmatchpatch.New() - diffs := dmp.DiffMain(string(lb), string(rb), true) - for _, d := range diffs { - if d.Type != diffmatchpatch.DiffEqual { - return dmp.DiffPrettyText(diffs) - } - } - return "" -} diff --git a/pkg/builder/interface.go b/pkg/builder/interface.go index 6a6fb671..53ba3881 100644 --- a/pkg/builder/interface.go +++ b/pkg/builder/interface.go @@ -29,9 +29,11 @@ type Operation interface { // Name provides the unique name for this operation, see OperationFromStatus. Name() string - // Checkpoint augments the provided BuildStatus with sufficient state to be restored - // by OperationFromStatus on an appropriate BuildProvider. - Checkpoint(*v1alpha1.BuildStatus) error + // Checkpoint augments the provided BuildStatus with sufficient state to be + // restored by OperationFromStatus on an appropriate BuildProvider. + // + // This takes into account necessary information about the provided Build. + Checkpoint(*v1alpha1.Build, *v1alpha1.BuildStatus) error // Wait blocks until the Operation completes, returning either a status for the build or an error. // TODO(mattmoor): This probably shouldn't be BuildStatus, but some sort of smaller-scope thing. diff --git a/pkg/builder/nop/builder.go b/pkg/builder/nop/builder.go index 45e4f264..5c5986cd 100644 --- a/pkg/builder/nop/builder.go +++ b/pkg/builder/nop/builder.go @@ -40,7 +40,7 @@ type operation struct { func (nb *operation) Name() string { return operationName } -func (nb *operation) Checkpoint(status *v1alpha1.BuildStatus) error { +func (nb *operation) Checkpoint(_ *v1alpha1.Build, status *v1alpha1.BuildStatus) error { // Masquerade as the Google builder. status.Builder = v1alpha1.GoogleBuildProvider if status.Google == nil { diff --git a/pkg/builder/nop/builder_test.go b/pkg/builder/nop/builder_test.go index 8dc02555..9d6a5954 100644 --- a/pkg/builder/nop/builder_test.go +++ b/pkg/builder/nop/builder_test.go @@ -34,25 +34,27 @@ func TestBasicFlow(t *testing.T) { t.Fatalf("Unexpected error executing builder.Build: %v", err) } - var bs v1alpha1.BuildStatus - if err := op.Checkpoint(&bs); err != nil { + build := &v1alpha1.Build{ + Status: v1alpha1.BuildStatus{}, + } + if err := op.Checkpoint(build, &build.Status); err != nil { t.Fatalf("Unexpected error executing op.Checkpoint: %v", err) } - if buildercommon.IsDone(&bs) { - t.Errorf("IsDone(%v); wanted not done, got done.", bs) + if buildercommon.IsDone(&build.Status) { + t.Errorf("IsDone(%v); wanted not done, got done.", build.Status) } - op, err = builder.OperationFromStatus(&bs) + op, err = builder.OperationFromStatus(&build.Status) if err != nil { t.Fatalf("Unexpected error executing OperationFromStatus: %v", err) } - if bs.CreationTime.IsZero() { - t.Errorf("bs.CreationTime; want non-zero, got %v", bs.CreationTime) + if build.Status.CreationTime.IsZero() { + t.Errorf("build.Status.CreationTime; want non-zero, got %v", build.Status.CreationTime) } - if bs.StartTime.IsZero() { - t.Errorf("bs.StartTime; want non-zero, got %v", bs.StartTime) + if build.Status.StartTime.IsZero() { + t.Errorf("build.Status.StartTime; want non-zero, got %v", build.Status.StartTime) } - if !bs.CompletionTime.IsZero() { - t.Errorf("bs.CompletionTime; want zero, got %v", bs.CompletionTime) + if !build.Status.CompletionTime.IsZero() { + t.Errorf("build.Status.CompletionTime; want zero, got %v", build.Status.CompletionTime) } status, err := op.Wait() diff --git a/pkg/buildtest/diff.go b/pkg/buildtest/diff.go new file mode 100644 index 00000000..c07a7046 --- /dev/null +++ b/pkg/buildtest/diff.go @@ -0,0 +1,40 @@ +/* +Copyright 2018 The Knative 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 buildtest + +import ( + "encoding/json" + + "github.com/sergi/go-diff/diffmatchpatch" +) + +func JSONDiff(l, r interface{}) string { + lb, err := json.MarshalIndent(l, "", " ") + if err != nil { + panic(err.Error()) + } + rb, err := json.MarshalIndent(r, "", " ") + if err != nil { + panic(err.Error()) + } + + dmp := diffmatchpatch.New() + diffs := dmp.DiffMain(string(lb), string(rb), true) + for _, d := range diffs { + if d.Type != diffmatchpatch.DiffEqual { + return dmp.DiffPrettyText(diffs) + } + } + return "" +} diff --git a/pkg/controller/build/controller.go b/pkg/controller/build/controller.go index b2fcc4c0..15634aa9 100644 --- a/pkg/controller/build/controller.go +++ b/pkg/controller/build/controller.go @@ -362,7 +362,7 @@ func (c *Controller) syncHandler(key string) error { } return err } - if err := op.Checkpoint(&build.Status); err != nil { + if err := op.Checkpoint(build, &build.Status); err != nil { return err } build, err = c.updateStatus(build)