Improve validation message on build spec update

Author: csrwng

pkg/build/api/validation/validation.go

@@ -7,8 +7,12 @@ import (
 	"path/filepath"
 	"strings"
 
+	"github.com/golang/glog"
+
 	kapi "k8s.io/kubernetes/pkg/api"
 	"k8s.io/kubernetes/pkg/api/validation"
+	"k8s.io/kubernetes/pkg/runtime"
+	"k8s.io/kubernetes/pkg/util/strategicpatch"
 	kvalidation "k8s.io/kubernetes/pkg/util/validation"
 	"k8s.io/kubernetes/pkg/util/validation/field"
 
@@ -36,12 +40,43 @@ func ValidateBuildUpdate(build *buildapi.Build, older *buildapi.Build) field.Err
 		allErrs = append(allErrs, field.Invalid(field.NewPath("status", "phase"), build.Status.Phase, "phase cannot be updated from a terminal state"))
 	}
 	if !kapi.Semantic.DeepEqual(build.Spec, older.Spec) {
-		allErrs = append(allErrs, field.Invalid(field.NewPath("spec"), "content of spec is not printed out, please refer to the \"details\"", "spec is immutable"))
+		diff, err := diffBuildSpec(build.Spec, older.Spec)
+		if err != nil {
+			glog.V(2).Infof("Error calculating build spec patch: %v", err)
+			diff = "[unknown]"
+		}
+		detail := fmt.Sprintf("spec is immutable, diff: %s", diff)
+		allErrs = append(allErrs, field.Invalid(field.NewPath("spec"), "content of spec is not printed out, please refer to the details", detail))
 	}
 
 	return allErrs
 }
 
+func diffBuildSpec(newer buildapi.BuildSpec, older buildapi.BuildSpec) (string, error) {
+	version := buildapi.SchemeGroupVersion
+	codec := kapi.Codecs.LegacyCodec(version)
+	newerObj := &buildapi.Build{Spec: newer}
+	olderObj := &buildapi.Build{Spec: older}
+
+	newerJSON, err := runtime.Encode(codec, newerObj)
+	if err != nil {
+		return "", fmt.Errorf("error encoding newer: %v", err)
+	}
+	olderJSON, err := runtime.Encode(codec, olderObj)
+	if err != nil {
+		return "", fmt.Errorf("error encoding older: %v", err)
+	}
+	versioned, err := kapi.Scheme.ConvertToVersion(newerObj, version.String())
+	if err != nil {
+		return "", fmt.Errorf("error converting newer to versioned: %v", err)
+	}
+	patch, err := strategicpatch.CreateTwoWayMergePatch(olderJSON, newerJSON, versioned)
+	if err != nil {
+		return "", fmt.Errorf("error creating a strategic patch: %v", err)
+	}
+	return string(patch), nil
+}
+
 // refKey returns a key for the given ObjectReference. If the ObjectReference
 // doesn't include a namespace, the passed in namespace is used for the reference
 func refKey(namespace string, ref *kapi.ObjectReference) string {

pkg/build/api/validation/validation_test.go

@@ -9,6 +9,8 @@ import (
 	"k8s.io/kubernetes/pkg/util/validation/field"
 
 	buildapi "github.com/openshift/origin/pkg/build/api"
+
+	_ "github.com/openshift/origin/pkg/build/api/install"
 )
 
 func TestBuildValidationSuccess(t *testing.T) {
@@ -2320,3 +2322,79 @@ func TestValidatePostCommit(t *testing.T) {
 		}
 	}
 }
+
+func TestDiffBuildSpec(t *testing.T) {
+	tests := []struct {
+		name         string
+		older, newer buildapi.BuildSpec
+		expected     string
+	}{
+		{
+			older: buildapi.BuildSpec{
+				CommonSpec: buildapi.CommonSpec{
+					Source: buildapi.BuildSource{},
+				},
+			},
+			newer: buildapi.BuildSpec{
+				CommonSpec: buildapi.CommonSpec{
+					Source: buildapi.BuildSource{
+						ContextDir: "context-dir",
+					},
+				},
+			},
+			expected: `{"Spec":{"Source":{"ContextDir":"context-dir"}}}`,
+		},
+		{
+			older: buildapi.BuildSpec{
+				CommonSpec: buildapi.CommonSpec{
+					Source: buildapi.BuildSource{
+						Git: &buildapi.GitBuildSource{
+							Ref: "https://github.com/openshift/origin.git",
+						},
+					},
+				},
+			},
+			newer: buildapi.BuildSpec{
+				CommonSpec: buildapi.CommonSpec{
+					Source: buildapi.BuildSource{
+						Git: &buildapi.GitBuildSource{
+							Ref: "https://github.com/openshift/origin.git",
+						},
+					},
+				},
+			},
+			expected: "{}",
+		},
+		{
+			older: buildapi.BuildSpec{
+				CommonSpec: buildapi.CommonSpec{
+					Source: buildapi.BuildSource{
+						Git: &buildapi.GitBuildSource{
+							Ref: "https://github.com/openshift/origin.git",
+						},
+					},
+				},
+			},
+			newer: buildapi.BuildSpec{
+				CommonSpec: buildapi.CommonSpec{
+					Source: buildapi.BuildSource{
+						Git: &buildapi.GitBuildSource{
+							Ref: "https://github.com/ose/origin.git",
+						},
+					},
+				},
+			},
+			expected: `{"Spec":{"Source":{"Git":{"Ref":"https://github.com/ose/origin.git"}}}}`,
+		},
+	}
+	for _, test := range tests {
+		diff, err := diffBuildSpec(test.newer, test.older)
+		if err != nil {
+			t.Errorf("%s: unexpected: %v", test.name, err)
+			continue
+		}
+		if diff != test.expected {
+			t.Errorf("%s: expected: %s, got: %s", test.name, test.expected, diff)
+		}
+	}
+}