chore: adjust base on comments

pkg/resolution/resource/crd_resource.go

@@ -21,8 +21,8 @@ import (
 	"encoding/base64"
 	"errors"
 	"fmt"
+	"reflect"
 
-	"github.com/google/go-cmp/cmp"
 	pipelinev1beta1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
 	"github.com/tektoncd/pipeline/pkg/apis/resolution/v1beta1"
 	rrclient "github.com/tektoncd/pipeline/pkg/client/resolution/clientset/versioned"
@@ -124,7 +124,7 @@ func appendOwnerReference(rr *v1beta1.ResolutionRequest, req Request) {
 
 func ownerRefsAreEqual(a, b metav1.OwnerReference) bool {
 	// pointers values cannot be directly compared.
-	if d := cmp.Diff(a.Controller, b.Controller); d != "" {
+	if !reflect.DeepEqual(a.Controller, b.Controller) {
 		return false
 	}
 	return a.APIVersion == b.APIVersion && a.Kind == b.Kind && a.Name == b.Name && a.UID == b.UID

pkg/resolution/resource/crd_resource_test.go

@@ -20,7 +20,6 @@ import (
 	"context"
 	"encoding/base64"
 	"errors"
-	"reflect"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -30,7 +29,6 @@ import (
 	resolutioncommon "github.com/tektoncd/pipeline/pkg/resolution/common"
 	"github.com/tektoncd/pipeline/test"
 	"github.com/tektoncd/pipeline/test/diff"
-	"github.com/tektoncd/pipeline/test/names"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	duckv1 "knative.dev/pkg/apis/duck/v1"
 	"knative.dev/pkg/logging"
@@ -41,7 +39,6 @@ import (
 // d, where d represents the state of the system (existing resources) needed for the test.
 func getCRDRequester(t *testing.T, d test.Data) (test.Assets, func()) {
 	t.Helper()
-	names.TestingSeed()
 	return initializeCRDRequesterAssets(t, d)
 }
 
@@ -60,48 +57,54 @@ func initializeCRDRequesterAssets(t *testing.T, d test.Data) (test.Assets, func(
 }
 
 func TestCRDRequesterSubmit(t *testing.T) {
+	request := getRequestFromFile("testdata/request.golden.yaml")
 	testCases := []struct {
-		name                     string
-		inputRequest             *v1beta1.ResolutionRequest
-		expectedRequest          *v1beta1.ResolutionRequest
-		expectedResolvedResource *readOnlyResolutionRequest
-		expectedErr              error
+		name                      string
+		inputRequest              *test.RawRequest
+		inputResolutionRequest    *v1beta1.ResolutionRequest
+		expectedResolutionRequest *v1beta1.ResolutionRequest
+		expectedResolvedResource  *readOnlyResolutionRequest
+		expectedErr               error
 	}{
 		{
-			name:                     "resolution request does not exist and needs to be created",
-			inputRequest:             nil,
-			expectedRequest:          getResolutionRequestFromFile("testdata/submit.resolution_request.created.yaml"),
-			expectedResolvedResource: nil,
-			expectedErr:              resolutioncommon.ErrRequestInProgress,
+			name:                      "resolution request does not exist and needs to be created",
+			inputRequest:              request,
+			inputResolutionRequest:    nil,
+			expectedResolutionRequest: getResolutionRequestFromFile("testdata/submit.resolution_request.created.yaml"),
+			expectedResolvedResource:  nil,
+			expectedErr:               resolutioncommon.ErrRequestInProgress,
 		},
 		{
-			name:                     "resolution request exist and status is unknown",
-			inputRequest:             getResolutionRequestFromFile("testdata/submit.resolution_request.unknown.yaml"),
-			expectedRequest:          nil,
-			expectedResolvedResource: nil,
-			expectedErr:              resolutioncommon.ErrRequestInProgress,
+			name:                      "resolution request exist and status is unknown",
+			inputRequest:              request,
+			inputResolutionRequest:    getResolutionRequestFromFile("testdata/submit.resolution_request.unknown.yaml"),
+			expectedResolutionRequest: nil,
+			expectedResolvedResource:  nil,
+			expectedErr:               resolutioncommon.ErrRequestInProgress,
 		},
 		{
-			name:                     "resolution request exist and status is true",
-			inputRequest:             getResolutionRequestFromFile("testdata/submit.resolution_request.true.yaml"),
-			expectedRequest:          nil,
-			expectedResolvedResource: &readOnlyResolutionRequest{getResolutionRequestFromFile("testdata/submit.resolution_request.true.yaml")},
-			expectedErr:              nil,
+			name:                      "resolution request exist and status is succeeded",
+			inputRequest:              request,
+			inputResolutionRequest:    getResolutionRequestFromFile("testdata/submit.resolution_request.true.yaml"),
+			expectedResolutionRequest: nil,
+			expectedResolvedResource:  &readOnlyResolutionRequest{getResolutionRequestFromFile("testdata/submit.resolution_request.true.yaml")},
+			expectedErr:               nil,
 		},
 		{
-			name:                     "resolution request exist and status is false",
-			inputRequest:             getResolutionRequestFromFile("testdata/submit.resolution_request.false.yaml"),
-			expectedRequest:          nil,
-			expectedResolvedResource: nil,
-			expectedErr:              resolutioncommon.NewError(resolutioncommon.ReasonResolutionFailed, errors.New("message")),
+			name:                      "resolution request exist and status is failed",
+			inputRequest:              request,
+			inputResolutionRequest:    getResolutionRequestFromFile("testdata/submit.resolution_request.false.yaml"),
+			expectedResolutionRequest: nil,
+			expectedResolvedResource:  nil,
+			expectedErr:               resolutioncommon.NewError(resolutioncommon.ReasonResolutionFailed, errors.New("message")),
 		},
 	}
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
 			d := test.Data{}
-			if tc.inputRequest != nil {
-				d.ResolutionRequests = []*v1beta1.ResolutionRequest{tc.inputRequest}
+			if tc.inputResolutionRequest != nil {
+				d.ResolutionRequests = []*v1beta1.ResolutionRequest{tc.inputResolutionRequest}
 			}
 
 			testAssets, cancel := getCRDRequester(t, d)
@@ -110,39 +113,36 @@ func TestCRDRequesterSubmit(t *testing.T) {
 			clients := testAssets.Clients
 
 			resolver := ResolverName("git")
-			request := getRequestFromFile("testdata/request.golden.yaml")
 			crdRequester := NewCRDRequester(clients.ResolutionRequests, testAssets.Informers.ResolutionRequest.Lister())
-			resolvedResource, err := crdRequester.Submit(ctx, resolver, request.Request())
+			resolvedResource, err := crdRequester.Submit(ctx, resolver, tc.inputRequest.Request())
 
 			// check the error
-			if (tc.expectedErr != nil && err == nil) ||
-				(tc.expectedErr == nil && err != nil) {
-				t.Fatalf("expected error %v, but got %v", tc.expectedErr, err)
-			} else if tc.expectedErr != nil {
-				if d := cmp.Diff(tc.expectedErr.Error(), err.Error()); d != "" {
-					t.Fatalf("expected error to match %s", diff.PrintWantGot(d))
+			if err != nil || tc.expectedErr != nil {
+				if err == nil || tc.expectedErr == nil {
+					t.Errorf("expected error %v, but got %v", tc.expectedErr, err)
+				} else if err.Error() != tc.expectedErr.Error() {
+					t.Errorf("expected error %v, but got %v", tc.expectedErr, err)
 				}
 			}
 
 			// check the resolved resource
-			if (tc.expectedResolvedResource != nil && resolvedResource == nil) ||
-				(tc.expectedResolvedResource == nil && resolvedResource != nil) {
-				t.Fatalf("expected resolved resource equal %v, but got %v", tc.expectedResolvedResource, resolvedResource)
-			} else if tc.expectedResolvedResource != nil {
-				if d := cmp.Diff(tc.expectedResolvedResource.req, resolvedResource.(readOnlyResolutionRequest).req); d != "" {
-					t.Fatalf("expected resolved resource to match %s", diff.PrintWantGot(d))
+			if tc.expectedResolvedResource != nil || resolvedResource != nil {
+				if tc.expectedResolvedResource == nil || resolvedResource == nil {
+					t.Errorf("expected resolved resource equal %v, but got %v", tc.expectedResolvedResource, resolvedResource)
+				} else if d := cmp.Diff(tc.expectedResolvedResource.req, resolvedResource.(readOnlyResolutionRequest).req); d != "" {
+					t.Errorf("expected resolved resource to match %s", diff.PrintWantGot(d))
 				}
 			}
 
 			// check the resolution request
-			if tc.expectedRequest != nil {
+			if tc.expectedResolutionRequest != nil {
 				resolutionrequest, err := clients.ResolutionRequests.ResolutionV1beta1().
-					ResolutionRequests(request.Namespace).Get(ctx, request.Name, metav1.GetOptions{})
+					ResolutionRequests(tc.inputRequest.Namespace).Get(ctx, tc.inputRequest.Name, metav1.GetOptions{})
 				if err != nil {
-					t.Fatalf("unexpected error getting resource requests: %v", err)
+					t.Errorf("unexpected error getting resource requests: %v", err)
 				}
-				if d := cmp.Diff(tc.expectedRequest, resolutionrequest); d != "" {
-					t.Fatalf("expected resolution request to match %s", diff.PrintWantGot(d))
+				if d := cmp.Diff(tc.expectedResolutionRequest, resolutionrequest); d != "" {
+					t.Errorf("expected resolution request to match %s", diff.PrintWantGot(d))
 				}
 			}
 		})
@@ -167,7 +167,6 @@ func getOwnerRefFromFile(filepath string) *metav1.OwnerReference {
 func TestAppendOwnerReference(t *testing.T) {
 	request := getRequestFromFile("testdata/request.golden.yaml")
 	ownerRef := getOwnerRefFromFile("testdata/ownerref.golden.yaml")
-	logging.FromContext(context.TODO()).Infof("ownerRef: %v", ownerRef)
 	testCases := []struct {
 		name                      string
 		inputRequest              Request
@@ -177,13 +176,13 @@ func TestAppendOwnerReference(t *testing.T) {
 		{
 			name:                      "request does not implement OwnedRequest",
 			inputRequest:              request.Request(),
-			inputResolutionRequest:    getResolutionRequestFromFile("testdata/append.owner.empty.orignal.yaml"),
-			expectedResolutionRequest: getResolutionRequestFromFile("testdata/append.owner.empty.orignal.yaml"),
+			inputResolutionRequest:    getResolutionRequestFromFile("testdata/append.owner.empty.original.yaml"),
+			expectedResolutionRequest: getResolutionRequestFromFile("testdata/append.owner.empty.original.yaml"),
 		},
 		{
 			name:                      "request implements OwnedRequest but has no owner reference",
 			inputRequest:              &ownerRequest{request.Request(), *ownerRef},
-			inputResolutionRequest:    getResolutionRequestFromFile("testdata/append.owner.empty.orignal.yaml"),
+			inputResolutionRequest:    getResolutionRequestFromFile("testdata/append.owner.empty.original.yaml"),
 			expectedResolutionRequest: getResolutionRequestFromFile("testdata/append.owner.empty.expected.yaml"),
 		},
 		{
@@ -195,7 +194,7 @@ func TestAppendOwnerReference(t *testing.T) {
 		{
 			name:                      "request implements OwnedRequest but has the different owner reference",
 			inputRequest:              &ownerRequest{request.Request(), *ownerRef},
-			inputResolutionRequest:    getResolutionRequestFromFile("testdata/append.owner.different.orignal.yaml"),
+			inputResolutionRequest:    getResolutionRequestFromFile("testdata/append.owner.different.original.yaml"),
 			expectedResolutionRequest: getResolutionRequestFromFile("testdata/append.owner.different.expected.yaml"),
 		},
 	}
@@ -204,7 +203,7 @@ func TestAppendOwnerReference(t *testing.T) {
 		t.Run(tc.name, func(t *testing.T) {
 			appendOwnerReference(tc.inputResolutionRequest, tc.inputRequest)
 			if d := cmp.Diff(tc.expectedResolutionRequest, tc.inputResolutionRequest); d != "" {
-				t.Fatalf("expected resolution request to match %s", diff.PrintWantGot(d))
+				t.Errorf("expected resolution request to match %s", diff.PrintWantGot(d))
 			}
 		})
 	}
@@ -219,42 +218,120 @@ func (r *ownerRequest) OwnerRef() metav1.OwnerReference {
 	return r.ownerRef
 }
 
-func TestReadOnlyResolutionRequest(t *testing.T) {
-	rr := &v1beta1.ResolutionRequest{
+func TestReadOnlyResolutionRequest_Annotations(t *testing.T) {
+	testCases := []struct {
+		name                string
+		inputAnnotations    map[string]string
+		expectedAnnotations map[string]string
+	}{
+		{
+			name:                "no annotations",
+			inputAnnotations:    nil,
+			expectedAnnotations: nil,
+		},
+		{
+			name:                "some annotations",
+			inputAnnotations:    map[string]string{"key1": "val1", "key2": "val2"},
+			expectedAnnotations: map[string]string{"key1": "val1", "key2": "val2"},
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			readOnlyRR := crdIntoResource(generateResolutionRequest(tc.inputAnnotations, nil, nil))
+			if d := cmp.Diff(tc.expectedAnnotations, readOnlyRR.Annotations()); d != "" {
+				t.Errorf("expected annotations to match %s", diff.PrintWantGot(d))
+			}
+		})
+	}
+}
+
+func TestReadOnlyResolutionRequest_Data(t *testing.T) {
+	testCases := []struct {
+		name         string
+		inputData    []byte
+		invalidData  bool
+		expectedData []byte
+		expectedErr  error
+	}{
+		{
+			name:         "no data",
+			inputData:    nil,
+			expectedData: []byte{},
+			expectedErr:  nil,
+		},
+		{
+			name:         "some data",
+			inputData:    []byte("some data"),
+			expectedData: []byte("some data"),
+			expectedErr:  nil,
+		},
+		{
+			name:         "invalid data",
+			inputData:    []byte("WvLTlMrX9NpYDQlEIFlnDB"),
+			invalidData:  true,
+			expectedErr:  errors.New("error decoding data from base64: illegal base64 data at input byte 20"),
+			expectedData: nil,
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			rr := generateResolutionRequest(nil, tc.inputData, nil)
+			if tc.invalidData {
+				rr.Status.Data = string(tc.inputData)
+			}
+			data, err := crdIntoResource(rr).Data()
+			if err != nil || tc.expectedErr != nil {
+				if err == nil || tc.expectedErr == nil {
+					t.Errorf("expected err to be %v, got %v", tc.expectedErr, err)
+				} else if tc.expectedErr.Error() != err.Error() {
+					t.Errorf("expected err to be %v, got %v", tc.expectedErr, err)
+				}
+			}
+			if d := cmp.Diff(tc.expectedData, data); d != "" {
+				t.Errorf("expected data to match %s", diff.PrintWantGot(d))
+			}
+		})
+	}
+}
+
+func TestReadOnlyResolutionRequest_Source(t *testing.T) {
+	testCases := []struct {
+		name           string
+		inputSource    *pipelinev1beta1.ConfigSource
+		expectedSource *pipelinev1beta1.ConfigSource
+	}{
+		{
+			name:           "no source",
+			inputSource:    nil,
+			expectedSource: nil,
+		},
+		{
+			name:           "some source",
+			inputSource:    &pipelinev1beta1.ConfigSource{URI: "uri"},
+			expectedSource: &pipelinev1beta1.ConfigSource{URI: "uri"},
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			readOnlyRR := crdIntoResource(generateResolutionRequest(nil, nil, tc.inputSource))
+			if d := cmp.Diff(tc.expectedSource, readOnlyRR.Source()); d != "" {
+				t.Errorf("expected source to match %s", diff.PrintWantGot(d))
+			}
+		})
+	}
+}
+
+func generateResolutionRequest(annotation map[string]string, data []byte,
+	source *pipelinev1beta1.ConfigSource) *v1beta1.ResolutionRequest {
+	return &v1beta1.ResolutionRequest{
 		Status: v1beta1.ResolutionRequestStatus{
 			Status: duckv1.Status{
-				Annotations: map[string]string{"key1": "val1", "key2": "val2"},
+				Annotations: annotation,
 			},
 			ResolutionRequestStatusFields: v1beta1.ResolutionRequestStatusFields{
-				Data:   base64.StdEncoding.EncodeToString([]byte("some data")),
-				Source: &pipelinev1beta1.ConfigSource{URI: "uri"},
+				Data:   base64.StdEncoding.EncodeToString(data),
+				Source: source,
 			},
 		},
 	}
-	readOnlyRR := crdIntoResource(rr)
-
-	t.Run("Annotations", func(t *testing.T) {
-		expectedAnnotations := map[string]string{"key1": "val1", "key2": "val2"}
-		if !reflect.DeepEqual(readOnlyRR.Annotations(), expectedAnnotations) {
-			t.Errorf("Annotations() returned unexpected value, got: %v, want: %v", readOnlyRR.Annotations(), expectedAnnotations)
-		}
-	})
-
-	t.Run("Data", func(t *testing.T) {
-		expectedData := []byte("some data")
-		data, err := readOnlyRR.Data()
-		if err != nil {
-			t.Errorf("Data() returned unexpected error: %v", err)
-		}
-		if !reflect.DeepEqual(data, expectedData) {
-			t.Errorf("Data() returned unexpected value, got: %v, want: %v", data, expectedData)
-		}
-	})
-
-	t.Run("Source", func(t *testing.T) {
-		expectedSource := &pipelinev1beta1.ConfigSource{URI: "uri"}
-		if !reflect.DeepEqual(readOnlyRR.Source(), expectedSource) {
-			t.Errorf("Source() returned unexpected value, got: %v, want: %v", readOnlyRR.Source(), expectedSource)
-		}
-	})
 }

pkg/resolution/resource/testdata/submit.resolution_request.created.yaml

@@ -7,12 +7,9 @@ metadata:
     resolution.tekton.dev/type: "git"
 spec:
   params:
-    -
-      name: "url"
+    - name: "url"
       value: "https://github.com/tektoncd/catalog"
-    -
-      name: "revision"
+    - name: "revision"
       value: "main"
-    -
-      name: "pathInRepo"
+    - name: "pathInRepo"
       value: "task/git-clone/0.6/git-clone.yaml"