Skip to content

Commit

Permalink
Fix 404 error for GitLab provider
Browse files Browse the repository at this point in the history 
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 <sashture@redhat.com>
  • Loading branch information
@savitaashture @chmouel
savitaashture authored and chmouel committed Jun 4, 2024
1 parent 37e085b commit 6911e85
Show file tree
Hide file tree
Showing 3 changed files with 70 additions and 20 deletions.
1 change: 1 addition & 0 deletions pkg/customparams/standard.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
27 changes: 18 additions & 9 deletions pkg/provider/gitlab/gitlab.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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\n<small>Full log available [here](%s)</small>",
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() ||
Expand Down Expand Up @@ -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
}
Expand Down
62 changes: 51 additions & 11 deletions pkg/provider/gitlab/gitlab_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -542,6 +579,7 @@ func TestGetFiles(t *testing.T) {
wantDeletedFilesCount: 1,
wantModifiedFilesCount: 1,
wantRenamedFilesCount: 1,
sourceProjectID: 0,
},
}
for _, tt := range tests {
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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))
Expand Down

0 comments on commit 6911e85

Please sign in to comment.