Skip to content

fix(gitlab): use TargetProjectID for merge request comments #2194

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions pkg/provider/gitlab/gitlab.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ func (v *Provider) CreateComment(_ context.Context, event *info.Event, commit, u

// List comments of the merge request
if updateMarker != "" {
comments, _, err := v.Client().Notes.ListMergeRequestNotes(v.sourceProjectID, event.PullRequestNumber, &gitlab.ListMergeRequestNotesOptions{
comments, _, err := v.Client().Notes.ListMergeRequestNotes(event.TargetProjectID, event.PullRequestNumber, &gitlab.ListMergeRequestNotesOptions{
ListOptions: gitlab.ListOptions{
Page: 1,
PerPage: 100,
Expand All @@ -108,15 +108,15 @@ func (v *Provider) CreateComment(_ context.Context, event *info.Event, commit, u
re := regexp.MustCompile(updateMarker)
for _, comment := range comments {
if re.MatchString(comment.Body) {
_, _, err := v.Client().Notes.UpdateMergeRequestNote(v.sourceProjectID, event.PullRequestNumber, comment.ID, &gitlab.UpdateMergeRequestNoteOptions{
_, _, err := v.Client().Notes.UpdateMergeRequestNote(event.TargetProjectID, event.PullRequestNumber, comment.ID, &gitlab.UpdateMergeRequestNoteOptions{
Body: &commit,
})
return err
}
}
}

_, _, err := v.Client().Notes.CreateMergeRequestNote(v.sourceProjectID, event.PullRequestNumber, &gitlab.CreateMergeRequestNoteOptions{
_, _, err := v.Client().Notes.CreateMergeRequestNote(event.TargetProjectID, event.PullRequestNumber, &gitlab.CreateMergeRequestNoteOptions{
Body: &commit,
})

Expand Down
6 changes: 3 additions & 3 deletions pkg/provider/gitlab/gitlab_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -991,7 +991,7 @@ func TestGitLabCreateComment(t *testing.T) {
},
{
name: "create new comment",
event: &info.Event{PullRequestNumber: 123},
event: &info.Event{PullRequestNumber: 123, TargetProjectID: 666},
commit: "New Comment",
updateMarker: "",
mockResponses: map[string]func(rw http.ResponseWriter, _ *http.Request){
Expand All @@ -1004,7 +1004,7 @@ func TestGitLabCreateComment(t *testing.T) {
},
{
name: "update existing comment",
event: &info.Event{PullRequestNumber: 123},
event: &info.Event{PullRequestNumber: 123, TargetProjectID: 666},
commit: "Updated Comment",
updateMarker: "MARKER",
mockResponses: map[string]func(rw http.ResponseWriter, _ *http.Request){
Expand All @@ -1023,7 +1023,7 @@ func TestGitLabCreateComment(t *testing.T) {
},
{
name: "no matching comment creates new",
event: &info.Event{PullRequestNumber: 123},
event: &info.Event{PullRequestNumber: 123, TargetProjectID: 666},
commit: "New Comment",
updateMarker: "MARKER",
mockResponses: map[string]func(rw http.ResponseWriter, _ *http.Request){
Expand Down
128 changes: 128 additions & 0 deletions test/gitlab_merge_request_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -690,6 +690,134 @@ func TestGitlabMergeRequestOnUpdateAtAndLabelChange(t *testing.T) {
}
}

func TestGitlabMergeRequestValidationErrorsFromFork(t *testing.T) {
targetNS := names.SimpleNameGenerator.RestrictLengthWithRandomSuffix("pac-e2e-ns")
ctx := context.Background()
runcnx, opts, glprovider, err := tgitlab.Setup(ctx)
assert.NilError(t, err)
ctx, err = cctx.GetControllerCtxInfo(ctx, runcnx)
assert.NilError(t, err)
runcnx.Clients.Log.Info("Testing GitLab validation error commenting from fork scenario")

// Get the original project onboarded to PaC
originalProject, resp, err := glprovider.Client().Projects.GetProject(opts.ProjectID, nil)
assert.NilError(t, err)
if resp != nil && resp.StatusCode == http.StatusNotFound {
t.Errorf("Repository %d not found", opts.ProjectID)
}

err = tgitlab.CreateCRD(ctx, originalProject, runcnx, opts, targetNS, nil)
assert.NilError(t, err)

// Create a fork of the original project
forkName := names.SimpleNameGenerator.RestrictLengthWithRandomSuffix("pac-fork-test")
forkProject, _, err := glprovider.Client().Projects.ForkProject(opts.ProjectID, &clientGitlab.ForkProjectOptions{
Name: &forkName,
Path: &forkName,
})
assert.NilError(t, err)
runcnx.Clients.Log.Infof("Created fork project: %s (ID: %d) from original: %s (ID: %d)",
forkProject.PathWithNamespace, forkProject.ID, originalProject.PathWithNamespace, originalProject.ID)

// Cleanup fork when test finishes
defer func() {
runcnx.Clients.Log.Infof("Cleaning up fork project: %d", forkProject.ID)
_, err := glprovider.Client().Projects.DeleteProject(forkProject.ID, nil)
if err != nil {
runcnx.Clients.Log.Warnf("Failed to delete fork project: %v", err)
}
}()

// Wait sometime for fork to be fully ready
time.Sleep(3 * time.Second)
Comment on lines +731 to +732

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using a fixed time.Sleep to wait for the fork to be ready can lead to flaky tests. The GitLab API provides a way to check the status of a fork operation. It's better to poll the project's import_status until it becomes finished. This will make the test more robust.

	// Wait for fork to be ready by polling its import status
	const maxForkRetries = 24
	const forkRetryInterval = 5 * time.Second
	var p *clientGitlab.Project
	for i := 0; i < maxForkRetries; i++ {
		p, _, err = glprovider.Client().Projects.GetProject(forkProject.ID, nil)
		assert.NilError(t, err)
		if p.ImportStatus == "finished" {
			runcnx.Clients.Log.Info("Fork project is ready.")
			break
		}
		if i == maxForkRetries-1 {
			t.Fatalf("Fork of project did not finish in time, status: %s", p.ImportStatus)
		}
		runcnx.Clients.Log.Infof("Waiting for fork to be ready, current status: %s... (%d/%d)", p.ImportStatus, i+1, maxForkRetries)
		time.Sleep(forkRetryInterval)
	}


// Commit invalid .tekton files to the fork
entries, err := payload.GetEntries(map[string]string{
".tekton/bad-yaml.yaml": "testdata/failures/bad-yaml.yaml",
}, targetNS, forkProject.DefaultBranch,
triggertype.PullRequest.String(), map[string]string{})
assert.NilError(t, err)

targetRefName := names.SimpleNameGenerator.RestrictLengthWithRandomSuffix("pac-e2e-fork-test")
forkCloneURL, err := scm.MakeGitCloneURL(forkProject.WebURL, opts.UserName, opts.Password)
assert.NilError(t, err)

commitTitle := "Add invalid .tekton file from fork - " + targetRefName
scmOpts := &scm.Opts{
GitURL: forkCloneURL,
CommitTitle: commitTitle,
Log: runcnx.Clients.Log,
WebURL: forkProject.WebURL,
TargetRefName: targetRefName,
BaseRefName: originalProject.DefaultBranch,
}
_ = scm.PushFilesToRefGit(t, scmOpts, entries)
runcnx.Clients.Log.Infof("Pushed invalid .tekton files to fork branch: %s", targetRefName)

// Create merge request from fork to original project
mrTitle := "TestValidationErrorsFromFork - " + targetRefName
mrOptions := &clientGitlab.CreateMergeRequestOptions{
Title: &mrTitle,
SourceBranch: &targetRefName,
TargetBranch: &originalProject.DefaultBranch,
// Create MR on the target project (original), not the source (fork)
TargetProjectID: &originalProject.ID,
}

mr, _, err := glprovider.Client().MergeRequests.CreateMergeRequest(forkProject.ID, mrOptions)
assert.NilError(t, err)
runcnx.Clients.Log.Infof("Created merge request from fork to original: %s/-/merge_requests/%d",
originalProject.WebURL, mr.IID)

defer func() {
// Close the MR and clean up branch
runcnx.Clients.Log.Infof("Closing MR %d", mr.IID)
_, _, err := glprovider.Client().MergeRequests.UpdateMergeRequest(originalProject.ID, mr.IID,
&clientGitlab.UpdateMergeRequestOptions{StateEvent: clientGitlab.Ptr("close")})
if err != nil {
runcnx.Clients.Log.Warnf("Failed to close MR: %v", err)
}

runcnx.Clients.Log.Infof("Deleting branch %s from fork", targetRefName)
_, err = glprovider.Client().Branches.DeleteBranch(forkProject.ID, targetRefName)
if err != nil {
runcnx.Clients.Log.Warnf("Failed to delete branch: %v", err)
}
}()

runcnx.Clients.Log.Info("Waiting for webhook validation to process MR and post validation comment...")

maxLoop := 12 // Wait up to 72 seconds for webhook processing
foundValidationComment := false

for i := 0; i < maxLoop; i++ {
notes, _, err := glprovider.Client().Notes.ListMergeRequestNotes(originalProject.ID, mr.IID, nil)
assert.NilError(t, err)

for _, note := range notes {
// Look for the validation error comment that PaC should post via webhook
if regexp.MustCompile(`.*There are some errors in your PipelineRun template.*`).MatchString(note.Body) &&
regexp.MustCompile(`.*bad-yaml\.yaml.*yaml validation error.*`).MatchString(note.Body) {
runcnx.Clients.Log.Info("Found validation error comment on original project's MR!")
foundValidationComment = true

// Verify the comment format matches PaC's validation error format
assert.Assert(t, regexp.MustCompile(`\[!CAUTION\]`).MatchString(note.Body), "Comment should contain caution header")
break
}
}

if foundValidationComment {
break
}

runcnx.Clients.Log.Infof("Loop %d/%d: Waiting for webhook validation to post comment (testing TargetProjectID fix)", i+1, maxLoop)
time.Sleep(6 * time.Second)
}

assert.Assert(t, foundValidationComment, "Validation error comment should appear on original project's MR. ")
}

// Local Variables:
// compile-command: "go test -tags=e2e -v -run ^TestGitlabMergeRequest$"
// End:
Loading