Merge branch 'main' into fix-gitlab-pending-status-mapping

Author: zakisk

.golangci.yml

@@ -58,7 +58,7 @@ linters:
     - zerologlint
   settings:
     misspell:
-      ignore-words:
+      ignore-rules:
         - cancelled
       extra-words:
         - typo: "canceled"
@@ -72,6 +72,8 @@ linters:
         - fmt.Fprintln
         - (io.Closer).Close
         - updateConfigMap
+    exhaustive:
+      default-signifies-exhaustive: true
     gocritic:
       disabled-checks:
         - unlambda

pkg/pipelineascode/pipelineascode.go

@@ -318,9 +318,11 @@ func (p *PacRun) startPR(ctx context.Context, match matcher.Match) (*tektonv1.Pi
 	if len(patchAnnotations) > 0 || len(patchLabels) > 0 {
 		pr, err = action.PatchPipelineRun(ctx, p.logger, whatPatching, p.run.Clients.Tekton, pr, getMergePatch(patchAnnotations, patchLabels))
 		if err != nil {
-			// we still return the created PR with error, and allow caller to decide what to do with the PR, and avoid
-			// unneeded SIGSEGV's
-			return pr, fmt.Errorf("cannot patch pipelinerun %s: %w", pr.GetGenerateName(), err)
+			// if PipelineRun patch is failed then do not return error, just log the error
+			// because its a false negative and on startPR return a failed check is being created
+			// due to this.
+			p.logger.Errorf("cannot patch pipelinerun %s: %w", pr.GetGenerateName(), err)
+			return pr, nil
 		}
 		currentReason := ""
 		if len(pr.Status.GetConditions()) > 0 {

pkg/pipelineascode/pipelineascode_startpr_test.go

@@ -765,31 +765,29 @@ func TestStartPR_PatchBehavior(t *testing.T) {
 		patchErrorMsg       string
 		expectError         bool
 		expectErrorContains []string
+		expectedLogSnippet  string
 		verifyAnnotations   bool // whether to verify annotations were set correctly
 	}{
 		{
-			name:               "successful patch - all annotations set",
-			simulatePatchError: false,
-			expectError:        false,
-			verifyAnnotations:  true,
+			name:              "successful patch - all annotations set",
+			verifyAnnotations: true,
 		},
 		{
-			name:                "patch failure - PR still returned with error",
-			simulatePatchError:  true,
-			patchErrorMsg:       "etcd unavailable",
-			expectError:         true,
-			expectErrorContains: []string{"cannot patch pipelinerun", "etcd unavailable"},
-			verifyAnnotations:   false,
+			name:               "patch failure - PR still returned with error",
+			simulatePatchError: true,
+			patchErrorMsg:      "etcd unavailable",
+			expectedLogSnippet: "cannot patch pipelinerun",
+			verifyAnnotations:  false,
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
+			observer, log := zapobserver.New(zap.InfoLevel)
+			logger := zap.New(observer).Sugar()
 			if tt.simulatePatchError {
 				// Need custom reactor to simulate patch failure
 				ctx, _ := rtesting.SetupFakeContext(t)
-				observer, _ := zapobserver.New(zap.InfoLevel)
-				logger := zap.New(observer).Sugar()
 
 				stdata, _ := testclient.SeedTestData(t, ctx, testclient.Data{
 					Namespaces: []*corev1.Namespace{
@@ -889,6 +887,11 @@ func TestStartPR_PatchBehavior(t *testing.T) {
 				assert.NilError(t, err)
 				assert.Assert(t, pr != nil, "PipelineRun should be returned")
 
+				if tt.expectedLogSnippet != "" {
+					logmsg := log.FilterMessageSnippet(tt.expectedLogSnippet).TakeAll()
+					assert.Assert(t, len(logmsg) > 0, "log messages", logmsg, tt.expectedLogSnippet)
+				}
+
 				if tt.verifyAnnotations {
 					state, hasState := pr.GetAnnotations()[keys.State]
 					assert.Assert(t, hasState, "State annotation should be patched")

pkg/provider/bitbucketdatacenter/bitbucketdatacenter.go

@@ -42,6 +42,7 @@ type Provider struct {
 	projectKey                string
 	repo                      *v1alpha1.Repository
 	triggerEvent              string
+	cachedChangedFiles        *changedfiles.ChangedFiles
 }
 
 func (v Provider) Client() *scm.Client {
@@ -371,13 +372,28 @@ func (v *Provider) GetConfig() *info.ProviderConfig {
 	}
 }
 
+// GetFiles gets and caches the list of files changed by a given event.
 func (v *Provider) GetFiles(ctx context.Context, runevent *info.Event) (changedfiles.ChangedFiles, error) {
-	OrgAndRepo := fmt.Sprintf("%s/%s", runevent.Organization, runevent.Repository)
-	if runevent.TriggerTarget == triggertype.PullRequest {
+	if v.cachedChangedFiles == nil {
+		changes, err := v.fetchChangedFiles(ctx, runevent)
+		if err != nil {
+			return changedfiles.ChangedFiles{}, err
+		}
+		v.cachedChangedFiles = &changes
+	}
+	return *v.cachedChangedFiles, nil
+}
+
+func (v *Provider) fetchChangedFiles(ctx context.Context, runevent *info.Event) (changedfiles.ChangedFiles, error) {
+	changedFiles := changedfiles.ChangedFiles{}
+
+	orgAndRepo := fmt.Sprintf("%s/%s", runevent.Organization, runevent.Repository)
+
+	switch runevent.TriggerTarget {
+	case triggertype.PullRequest:
 		opts := &scm.ListOptions{Page: 1, Size: apiResponseLimit}
-		changedFiles := changedfiles.ChangedFiles{}
 		for {
-			changes, _, err := v.Client().PullRequests.ListChanges(ctx, OrgAndRepo, runevent.PullRequestNumber, opts)
+			changes, _, err := v.Client().PullRequests.ListChanges(ctx, orgAndRepo, runevent.PullRequestNumber, opts)
 			if err != nil {
 				return changedfiles.ChangedFiles{}, fmt.Errorf("failed to list changes for pull request: %w", err)
 			}
@@ -408,14 +424,10 @@ func (v *Provider) GetFiles(ctx context.Context, runevent *info.Event) (changedf
 
 			opts.Page++
 		}
-		return changedFiles, nil
-	}
-
-	if runevent.TriggerTarget == triggertype.Push {
+	case triggertype.Push:
 		opts := &scm.ListOptions{Page: 1, Size: apiResponseLimit}
-		changedFiles := changedfiles.ChangedFiles{}
 		for {
-			changes, _, err := v.Client().Git.ListChanges(ctx, OrgAndRepo, runevent.SHA, opts)
+			changes, _, err := v.Client().Git.ListChanges(ctx, orgAndRepo, runevent.SHA, opts)
 			if err != nil {
 				return changedfiles.ChangedFiles{}, fmt.Errorf("failed to list changes for commit %s: %w", runevent.SHA, err)
 			}
@@ -442,9 +454,10 @@ func (v *Provider) GetFiles(ctx context.Context, runevent *info.Event) (changedf
 
 			opts.Page++
 		}
-		return changedFiles, nil
+	default:
+		// No action necessary
 	}
-	return changedfiles.ChangedFiles{}, nil
+	return changedFiles, nil
 }
 
 func (v *Provider) CreateToken(_ context.Context, _ []string, _ *info.Event) (string, error) {

pkg/provider/bitbucketdatacenter/bitbucketdatacenter_test.go

@@ -22,11 +22,14 @@ import (
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/params/triggertype"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/provider"
 	bbtest "github.com/openshift-pipelines/pipelines-as-code/pkg/provider/bitbucketdatacenter/test"
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/test/metrics"
 
 	"github.com/jenkins-x/go-scm/scm"
 	"go.uber.org/zap"
 	zapobserver "go.uber.org/zap/zaptest/observer"
 	"gotest.tools/v3/assert"
+	"knative.dev/pkg/metrics/metricstest"
+	_ "knative.dev/pkg/metrics/testing"
 	rtesting "knative.dev/pkg/reconciler/testing"
 )
 
@@ -796,6 +799,7 @@ func TestGetFiles(t *testing.T) {
 			ctx, _ := rtesting.SetupFakeContext(t)
 			client, mux, tearDown, tURL := bbtest.SetupBBDataCenterClient()
 			defer tearDown()
+			metrics.ResetMetrics()
 
 			stats := &bbtest.DiffStats{
 				Values: tt.changeFiles,
@@ -821,13 +825,17 @@ func TestGetFiles(t *testing.T) {
 					}
 				})
 			}
-			v := &Provider{client: client, baseURL: tURL}
+
+			metricsTags := map[string]string{"provider": "bitbucket-datacenter", "event-type": string(tt.event.TriggerTarget)}
+			metricstest.CheckStatsNotReported(t, "pipelines_as_code_git_provider_api_request_count")
+
+			v := &Provider{client: client, baseURL: tURL, triggerEvent: string(tt.event.TriggerTarget)}
 			changedFiles, err := v.GetFiles(ctx, tt.event)
 			if tt.wantError {
 				assert.Equal(t, err.Error(), tt.errMsg)
-				return
+			} else {
+				assert.NilError(t, err, nil)
 			}
-			assert.NilError(t, err, nil)
 			assert.Equal(t, tt.wantAddedFilesCount, len(changedFiles.Added))
 			assert.Equal(t, tt.wantDeletedFilesCount, len(changedFiles.Deleted))
 			assert.Equal(t, tt.wantModifiedFilesCount, len(changedFiles.Modified))
@@ -844,6 +852,17 @@ func TestGetFiles(t *testing.T) {
 					assert.Equal(t, tt.changeFiles[i].Path.ToString, changedFiles.All[i])
 				}
 			}
+
+			// Check caching
+			metricstest.CheckCountData(t, "pipelines_as_code_git_provider_api_request_count", metricsTags, 1)
+			_, _ = v.GetFiles(ctx, tt.event)
+			if tt.wantError {
+				// No caching on error
+				metricstest.CheckCountData(t, "pipelines_as_code_git_provider_api_request_count", metricsTags, 2)
+			} else {
+				// Cache API results on success
+				metricstest.CheckCountData(t, "pipelines_as_code_git_provider_api_request_count", metricsTags, 1)
+			}
 		})
 	}
 }

pkg/provider/github/github.go

@@ -55,7 +55,8 @@ type Provider struct {
 	PaginedNumber int
 	userType      string // The type of user i.e bot or not
 	skippedRun
-	triggerEvent string
+	triggerEvent       string
+	cachedChangedFiles *changedfiles.ChangedFiles
 }
 
 type skippedRun struct {
@@ -518,11 +519,24 @@ func (v *Provider) getPullRequest(ctx context.Context, runevent *info.Event) (*i
 	return runevent, nil
 }
 
-// GetFiles get a files from pull request.
+// GetFiles gets and caches the list of files changed by a given event.
 func (v *Provider) GetFiles(ctx context.Context, runevent *info.Event) (changedfiles.ChangedFiles, error) {
-	if runevent.TriggerTarget == triggertype.PullRequest {
+	if v.cachedChangedFiles == nil {
+		changes, err := v.fetchChangedFiles(ctx, runevent)
+		if err != nil {
+			return changedfiles.ChangedFiles{}, err
+		}
+		v.cachedChangedFiles = &changes
+	}
+	return *v.cachedChangedFiles, nil
+}
+
+func (v *Provider) fetchChangedFiles(ctx context.Context, runevent *info.Event) (changedfiles.ChangedFiles, error) {
+	changedFiles := changedfiles.ChangedFiles{}
+
+	switch runevent.TriggerTarget {
+	case triggertype.PullRequest:
 		opt := &github.ListOptions{PerPage: v.PaginedNumber}
-		changedFiles := changedfiles.ChangedFiles{}
 		for {
 			repoCommit, resp, err := wrapAPI(v, "list_pull_request_files", func() ([]*github.CommitFile, *github.Response, error) {
 				return v.Client().PullRequests.ListFiles(ctx, runevent.Organization, runevent.Repository, runevent.PullRequestNumber, opt)
@@ -550,11 +564,7 @@ func (v *Provider) GetFiles(ctx context.Context, runevent *info.Event) (changedf
 			}
 			opt.Page = resp.NextPage
 		}
-		return changedFiles, nil
-	}
-
-	if runevent.TriggerTarget == "push" {
-		changedFiles := changedfiles.ChangedFiles{}
+	case triggertype.Push:
 		rC, _, err := wrapAPI(v, "get_commit_files", func() (*github.RepositoryCommit, *github.Response, error) {
 			return v.Client().Repositories.GetCommit(ctx, runevent.Organization, runevent.Repository, runevent.SHA, &github.ListOptions{})
 		})
@@ -576,9 +586,10 @@ func (v *Provider) GetFiles(ctx context.Context, runevent *info.Event) (changedf
 				changedFiles.Renamed = append(changedFiles.Renamed, *rC.Files[i].Filename)
 			}
 		}
-		return changedFiles, nil
+	default:
+		// No action necessary
 	}
-	return changedfiles.ChangedFiles{}, nil
+	return changedFiles, nil
 }
 
 // getObject Get an object from a repository.