From 6911e85a26ba52fd6145c7c0f9e548ff73ce0c3c Mon Sep 17 00:00:00 2001 From: savitaashture Date: Wed, 29 May 2024 12:35:24 +0530 Subject: [PATCH] Fix 404 error for GitLab provider In GitLab, when there is an MR from a forked repo to a target repo, it gives a 404 error from the GetMergeRequestChanges function This PR addresses the fix Signed-off-by: Savita Ashture --- pkg/customparams/standard.go | 1 + pkg/provider/gitlab/gitlab.go | 27 ++++++++----- pkg/provider/gitlab/gitlab_test.go | 62 ++++++++++++++++++++++++------ 3 files changed, 70 insertions(+), 20 deletions(-) diff --git a/pkg/customparams/standard.go b/pkg/customparams/standard.go index 957f16dfe..77981d8ba 100644 --- a/pkg/customparams/standard.go +++ b/pkg/customparams/standard.go @@ -17,6 +17,7 @@ func (p *CustomParams) getChangedFiles(ctx context.Context) changedfiles.Changed changedFiles, err := p.vcx.GetFiles(ctx, p.event) if err != nil { p.eventEmitter.EmitMessage(p.repo, zap.ErrorLevel, "ParamsError", fmt.Sprintf("error getting changed files: %s", err.Error())) + return changedfiles.ChangedFiles{} } changedFiles.RemoveDuplicates() return changedFiles diff --git a/pkg/provider/gitlab/gitlab.go b/pkg/provider/gitlab/gitlab.go index cb4c5c4e9..b7e649fdd 100644 --- a/pkg/provider/gitlab/gitlab.go +++ b/pkg/provider/gitlab/gitlab.go @@ -55,6 +55,8 @@ type Provider struct { pathWithNamespace string repoURL string apiURL string + eventEmitter *events.EventEmitter + repo *v1alpha1.Repository } func (v *Provider) SetPacInfo(pacInfo *info.PacOpts) { @@ -109,7 +111,7 @@ func (v *Provider) GetConfig() *info.ProviderConfig { } } -func (v *Provider) SetClient(_ context.Context, run *params.Run, runevent *info.Event, _ *v1alpha1.Repository, _ *events.EventEmitter) error { +func (v *Provider) SetClient(_ context.Context, run *params.Run, runevent *info.Event, repo *v1alpha1.Repository, eventsEmitter *events.EventEmitter) error { var err error if runevent.Provider.Token == "" { return fmt.Errorf("no git_provider.secret has been set in the repo crd") @@ -157,6 +159,8 @@ func (v *Provider) SetClient(_ context.Context, run *params.Run, runevent *info. runevent.DefaultBranch = projectinfo.DefaultBranch } v.run = run + v.eventEmitter = eventsEmitter + v.repo = repo return nil } @@ -198,19 +202,24 @@ func (v *Provider) CreateStatus(_ context.Context, event *info.Event, statusOpts body := fmt.Sprintf("**%s%s** has %s\n\n%s\n\nFull log available [here](%s)", v.pacInfo.ApplicationName, onPr, statusOpts.Title, statusOpts.Text, detailsURL) - // in case we have access set the commit status, typically on MR from - // another users we won't have it but it would work on push or MR from a - // branch on the same repo or if token somehow can have access by other - // means. - // if we have an error fallback to send a issue comment opt := &gitlab.SetCommitStatusOptions{ State: gitlab.BuildStateValue(statusOpts.Conclusion), Name: gitlab.Ptr(v.pacInfo.ApplicationName), TargetURL: gitlab.Ptr(detailsURL), Description: gitlab.Ptr(statusOpts.Title), } - //nolint: dogsled - _, _, _ = v.Client.Commits.SetCommitStatus(event.SourceProjectID, event.SHA, opt) + + // In case we have access, set the status. Typically, on a Merge Request (MR) + // from a fork in an upstream repository, the token needs to have write access + // to the fork repository in order to create a status. However, the token set on the + // Repository CR usually doesn't have such broad access, preventing from creating + // a status comment on it. + // This would work on a push or an MR from a branch within the same repo. + // Ignoring errors because of the write access issues, + if _, _, err := v.Client.Commits.SetCommitStatus(event.SourceProjectID, event.SHA, opt); err != nil { + v.eventEmitter.EmitMessage(v.repo, zap.ErrorLevel, "FailedToSetCommitStatus", + "cannot set status with the GitLab token because of: "+err.Error()) + } // only add a note when we are on a MR if event.EventType == triggertype.PullRequest.String() || @@ -328,7 +337,7 @@ func (v *Provider) GetFiles(_ context.Context, runevent *info.Event) (changedfil } if runevent.TriggerTarget == triggertype.PullRequest { //nolint: staticcheck - mrchanges, _, err := v.Client.MergeRequests.GetMergeRequestChanges(v.sourceProjectID, runevent.PullRequestNumber, &gitlab.GetMergeRequestChangesOptions{}) + mrchanges, _, err := v.Client.MergeRequests.GetMergeRequestChanges(v.targetProjectID, runevent.PullRequestNumber, &gitlab.GetMergeRequestChangesOptions{}) if err != nil { return changedfiles.ChangedFiles{}, err } diff --git a/pkg/provider/gitlab/gitlab_test.go b/pkg/provider/gitlab/gitlab_test.go index 148bcd833..ea48ca69c 100644 --- a/pkg/provider/gitlab/gitlab_test.go +++ b/pkg/provider/gitlab/gitlab_test.go @@ -472,14 +472,16 @@ func TestValidate(t *testing.T) { func TestGetFiles(t *testing.T) { tests := []struct { - name string - event *info.Event - mrchanges *gitlab.MergeRequest - pushChanges []*gitlab.Diff - wantAddedFilesCount int - wantDeletedFilesCount int - wantModifiedFilesCount int - wantRenamedFilesCount int + name string + event *info.Event + mrchanges *gitlab.MergeRequest + pushChanges []*gitlab.Diff + wantAddedFilesCount int + wantDeletedFilesCount int + wantModifiedFilesCount int + wantRenamedFilesCount int + sourceProjectID, targetProjectID int + wantError bool }{ { name: "pull-request", @@ -512,6 +514,41 @@ func TestGetFiles(t *testing.T) { wantDeletedFilesCount: 1, wantModifiedFilesCount: 1, wantRenamedFilesCount: 1, + targetProjectID: 10, + }, + { + name: "pull-request with wrong project ID", + event: &info.Event{ + TriggerTarget: "pull_request", + Organization: "pullrequestowner", + Repository: "pullrequestrepository", + PullRequestNumber: 10, + }, + mrchanges: &gitlab.MergeRequest{ + Changes: []*gitlab.MergeRequestDiff{ + { + NewPath: "modified.yaml", + }, + { + NewPath: "added.doc", + NewFile: true, + }, + { + NewPath: "removed.yaml", + DeletedFile: true, + }, + { + NewPath: "renamed.doc", + RenamedFile: true, + }, + }, + }, + wantAddedFilesCount: 0, + wantDeletedFilesCount: 0, + wantModifiedFilesCount: 0, + wantRenamedFilesCount: 0, + targetProjectID: 12, + wantError: true, }, { name: "push", @@ -542,6 +579,7 @@ func TestGetFiles(t *testing.T) { wantDeletedFilesCount: 1, wantModifiedFilesCount: 1, wantRenamedFilesCount: 1, + sourceProjectID: 0, }, } for _, tt := range tests { @@ -569,7 +607,7 @@ func TestGetFiles(t *testing.T) { }, } if tt.event.TriggerTarget == "pull_request" { - mux.HandleFunc(fmt.Sprintf("/projects/0/merge_requests/%d/changes", + mux.HandleFunc(fmt.Sprintf("/projects/10/merge_requests/%d/changes", tt.event.PullRequestNumber), func(rw http.ResponseWriter, _ *http.Request) { jeez, err := json.Marshal(mergeFileChanges) assert.NilError(t, err) @@ -602,9 +640,11 @@ func TestGetFiles(t *testing.T) { }) } - providerInfo := &Provider{Client: fakeclient} + providerInfo := &Provider{Client: fakeclient, sourceProjectID: tt.sourceProjectID, targetProjectID: tt.targetProjectID} changedFiles, err := providerInfo.GetFiles(ctx, tt.event) - assert.NilError(t, err, nil) + if tt.wantError != true { + 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))