Skip to content

Commit 071640b

Browse files
ab-ghoshab-ghosh
committed
fix(gitlab): pin commit statuses to same pipeline
 When PAC posts multiple commit statuses for the same SHA, GitLab's auto-assignment logic can route them to different pipelines, leaving the MR pipeline permanently stuck with stale intermediate statuses. Cache the pipeline_id returned by the first SetCommitStatus response and pass it on subsequent calls for the same (project, SHA) pair so all statuses land in the same GitLab pipeline. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Abhishek Ghosh <abghosh@redhat.com>
1 parent 4c43e22 commit 071640b

File tree

2 files changed

+341
-2
lines changed

2 files changed

+341
-2
lines changed

‎pkg/provider/gitlab/gitlab.go‎

Lines changed: 43 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,11 @@ type Provider struct {
6969
memberCache map[int64]bool
7070
cachedChangedFiles *changedfiles.ChangedFiles
7171
pacUserID int64 // user login used by PAC
72+
// commitStatusPipelineIDs caches the GitLab pipeline ID returned by
73+
// the first SetCommitStatus call for a given (projectID, SHA) pair so
74+
// that subsequent statuses for the same commit land in the same
75+
// pipeline instead of GitLab auto-creating a new one.
76+
commitStatusPipelineIDs map[string]int64
7277
}
7378

7479
var defaultGitlabListOptions = gitlab.ListOptions{
@@ -354,22 +359,30 @@ func (v *Provider) CreateStatus(ctx context.Context, event *info.Event, statusOp
354359
Context: gitlab.Ptr(contextName),
355360
}
356361

362+
// Reuse a previously cached pipeline ID for this (project, SHA) pair so
363+
// that all commit statuses land in the same GitLab pipeline instead of
364+
// GitLab potentially auto-creating a new "external" pipeline mid-stream.
365+
v.setOptPipelineID(opt, event.SourceProjectID, event.SHA)
366+
357367
// In case we have access, set the status. Typically, on a Merge Request (MR)
358368
// from a fork in an upstream repository, the token needs to have write access
359369
// to the fork repository in order to create a status. However, the token set on the
360370
// Repository CR usually doesn't have such broad access, preventing from creating
361371
// a status comment on it.
362372
// This would work on a push or an MR from a branch within the same repo.
363373
// Ignoring errors because of the write access issues,
364-
_, _, err := v.Client().Commits.SetCommitStatus(event.SourceProjectID, event.SHA, opt)
374+
commitStatus, _, err := v.Client().Commits.SetCommitStatus(event.SourceProjectID, event.SHA, opt)
365375
if err != nil {
366376
v.Logger.Debugf("cannot set status with the GitLab token on the source project: %v", err)
367377
} else {
378+
v.cachePipelineID(event.SourceProjectID, event.SHA, commitStatus)
368379
// we managed to set the status on the source repo, all good we are done
369380
v.Logger.Debugf("created commit status on source project ID %d", event.TargetProjectID)
370381
return nil
371382
}
372-
if _, _, err = v.Client().Commits.SetCommitStatus(event.TargetProjectID, event.SHA, opt); err == nil {
383+
v.setOptPipelineID(opt, event.TargetProjectID, event.SHA)
384+
if commitStatus, _, err = v.Client().Commits.SetCommitStatus(event.TargetProjectID, event.SHA, opt); err == nil {
385+
v.cachePipelineID(event.TargetProjectID, event.SHA, commitStatus)
373386
v.Logger.Debugf("created commit status on target project ID %d", event.TargetProjectID)
374387
// we managed to set the status on the target repo, all good we are done
375388
return nil
@@ -860,3 +873,31 @@ func (v *Provider) formatPipelineComment(sha string, status providerstatus.Statu
860873
return fmt.Sprintf("%s **%s: %s/%s for %s**\n\n%s\n\n<small>Full log available [here](%s)</small>",
861874
emoji, status.Title, v.pacInfo.ApplicationName, status.OriginalPipelineRunName, sha, status.Text, status.DetailsURL)
862875
}
876+
877+
// pipelineIDKey builds the cache key for commitStatusPipelineIDs.
878+
func pipelineIDKey(projectID int64, sha string) string {
879+
return fmt.Sprintf("%d:%s", projectID, sha)
880+
}
881+
882+
// cachePipelineID stores the pipeline ID from a successful SetCommitStatus
883+
// response so that later calls for the same (project, SHA) reuse it.
884+
func (v *Provider) cachePipelineID(projectID int64, sha string, cs *gitlab.CommitStatus) {
885+
if cs == nil || cs.PipelineID == 0 {
886+
return
887+
}
888+
if v.commitStatusPipelineIDs == nil {
889+
v.commitStatusPipelineIDs = make(map[string]int64)
890+
}
891+
v.commitStatusPipelineIDs[pipelineIDKey(projectID, sha)] = cs.PipelineID
892+
}
893+
894+
// setOptPipelineID sets PipelineID on the options if we already have a cached
895+
// pipeline ID for this (project, SHA) pair.
896+
func (v *Provider) setOptPipelineID(opt *gitlab.SetCommitStatusOptions, projectID int64, sha string) {
897+
if v.commitStatusPipelineIDs == nil {
898+
return
899+
}
900+
if pid, ok := v.commitStatusPipelineIDs[pipelineIDKey(projectID, sha)]; ok {
901+
opt.PipelineID = gitlab.Ptr(pid)
902+
}
903+
}

‎pkg/provider/gitlab/gitlab_test.go‎

Lines changed: 298 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -441,6 +441,304 @@ func TestCreateStatus(t *testing.T) {
441441
}
442442
}
443443

444+
func TestCreateStatusPipelineIDCaching(t *testing.T) {
445+
ctx, _ := rtesting.SetupFakeContext(t)
446+
log, _ := logger.GetLogger()
447+
stdata, _ := testclient.SeedTestData(t, ctx, testclient.Data{})
448+
run := &params.Run{
449+
Clients: clients.Clients{
450+
Kube: stdata.Kube,
451+
Log: log,
452+
},
453+
}
454+
455+
client, mux, tearDown := thelp.Setup(t)
456+
defer tearDown()
457+
458+
v := &Provider{
459+
run: params.New(),
460+
Logger: log,
461+
pacInfo: &info.PacOpts{
462+
Settings: settings.Settings{
463+
ApplicationName: settings.PACApplicationNameDefaultValue,
464+
},
465+
},
466+
eventEmitter: events.NewEventEmitter(run.Clients.Kube, log),
467+
}
468+
v.SetGitLabClient(client)
469+
470+
event := &info.Event{
471+
TriggerTarget: "pull_request",
472+
SourceProjectID: 500,
473+
TargetProjectID: 500,
474+
SHA: "deadbeef",
475+
PullRequestNumber: 1,
476+
}
477+
478+
var callCount int
479+
mux.HandleFunc(fmt.Sprintf("/projects/%d/statuses/%s", event.SourceProjectID, event.SHA),
480+
func(rw http.ResponseWriter, r *http.Request) {
481+
callCount++
482+
var reqBody map[string]any
483+
if err := json.NewDecoder(r.Body).Decode(&reqBody); err != nil {
484+
t.Fatalf("failed to decode request body: %v", err)
485+
}
486+
// First call should NOT include pipeline_id
487+
if callCount == 1 {
488+
_, hasPipelineID := reqBody["pipeline_id"]
489+
assert.Assert(t, !hasPipelineID,
490+
"first call should not have pipeline_id")
491+
}
492+
// Subsequent calls MUST include the cached pipeline_id
493+
if callCount > 1 {
494+
pid, ok := reqBody["pipeline_id"]
495+
assert.Assert(t, ok, "subsequent calls must include pipeline_id")
496+
pidFloat, ok := pid.(float64)
497+
assert.Assert(t, ok, "pipeline_id must be a number")
498+
assert.Equal(t, int64(pidFloat), int64(9999),
499+
"pipeline_id must match cached value")
500+
}
501+
rw.WriteHeader(http.StatusCreated)
502+
fmt.Fprint(rw, `{"id": 1, "pipeline_id": 9999}`)
503+
})
504+
505+
// First call — should cache pipeline_id from response
506+
statusOpts := providerstatus.StatusOpts{
507+
Conclusion: "success",
508+
OriginalPipelineRunName: "pr-1",
509+
}
510+
err := v.CreateStatus(ctx, event, statusOpts)
511+
assert.NilError(t, err)
512+
assert.Equal(t, callCount, 1)
513+
514+
// Verify the pipeline ID was cached
515+
key := pipelineIDKey(event.SourceProjectID, event.SHA)
516+
assert.Equal(t, v.commitStatusPipelineIDs[key], int64(9999))
517+
518+
// Second call — should reuse the cached pipeline_id
519+
statusOpts.OriginalPipelineRunName = "pr-2"
520+
err = v.CreateStatus(ctx, event, statusOpts)
521+
assert.NilError(t, err)
522+
assert.Equal(t, callCount, 2)
523+
}
524+
525+
func TestCreateStatusPipelineIDCachingTargetFallback(t *testing.T) {
526+
ctx, _ := rtesting.SetupFakeContext(t)
527+
log, _ := logger.GetLogger()
528+
stdata, _ := testclient.SeedTestData(t, ctx, testclient.Data{})
529+
run := &params.Run{
530+
Clients: clients.Clients{
531+
Kube: stdata.Kube,
532+
Log: log,
533+
},
534+
}
535+
536+
client, mux, tearDown := thelp.Setup(t)
537+
defer tearDown()
538+
539+
v := &Provider{
540+
targetProjectID: 600,
541+
run: params.New(),
542+
Logger: log,
543+
pacInfo: &info.PacOpts{
544+
Settings: settings.Settings{
545+
ApplicationName: settings.PACApplicationNameDefaultValue,
546+
},
547+
},
548+
eventEmitter: events.NewEventEmitter(run.Clients.Kube, log),
549+
}
550+
v.SetGitLabClient(client)
551+
552+
event := &info.Event{
553+
TriggerTarget: "pull_request",
554+
SourceProjectID: 404, // will fail
555+
TargetProjectID: 600,
556+
SHA: "aabb1122",
557+
PullRequestNumber: 1,
558+
}
559+
560+
// Source returns 404
561+
mux.HandleFunc(fmt.Sprintf("/projects/%d/statuses/%s", event.SourceProjectID, event.SHA),
562+
func(rw http.ResponseWriter, _ *http.Request) {
563+
rw.WriteHeader(http.StatusNotFound)
564+
fmt.Fprint(rw, `{"message": "404 Not Found"}`)
565+
})
566+
567+
var targetCallCount int
568+
mux.HandleFunc(fmt.Sprintf("/projects/%d/statuses/%s", event.TargetProjectID, event.SHA),
569+
func(rw http.ResponseWriter, r *http.Request) {
570+
targetCallCount++
571+
var reqBody map[string]any
572+
if err := json.NewDecoder(r.Body).Decode(&reqBody); err != nil {
573+
t.Fatalf("failed to decode request body: %v", err)
574+
}
575+
if targetCallCount == 1 {
576+
_, hasPipelineID := reqBody["pipeline_id"]
577+
assert.Assert(t, !hasPipelineID,
578+
"first target call should not have pipeline_id")
579+
}
580+
if targetCallCount > 1 {
581+
pid, ok := reqBody["pipeline_id"]
582+
assert.Assert(t, ok, "subsequent target calls must include pipeline_id")
583+
pidFloat, ok := pid.(float64)
584+
assert.Assert(t, ok, "pipeline_id must be a number")
585+
assert.Equal(t, int64(pidFloat), int64(7777))
586+
}
587+
rw.WriteHeader(http.StatusCreated)
588+
fmt.Fprint(rw, `{"id": 1, "pipeline_id": 7777}`)
589+
})
590+
591+
statusOpts := providerstatus.StatusOpts{
592+
Conclusion: "success",
593+
OriginalPipelineRunName: "pr-1",
594+
}
595+
err := v.CreateStatus(ctx, event, statusOpts)
596+
assert.NilError(t, err)
597+
598+
key := pipelineIDKey(event.TargetProjectID, event.SHA)
599+
assert.Equal(t, v.commitStatusPipelineIDs[key], int64(7777))
600+
601+
// Second call should reuse target's cached pipeline_id
602+
statusOpts.OriginalPipelineRunName = "pr-2"
603+
err = v.CreateStatus(ctx, event, statusOpts)
604+
assert.NilError(t, err)
605+
assert.Equal(t, targetCallCount, 2)
606+
}
607+
608+
func TestCreateStatusPipelineIDZeroNotCached(t *testing.T) {
609+
ctx, _ := rtesting.SetupFakeContext(t)
610+
log, _ := logger.GetLogger()
611+
stdata, _ := testclient.SeedTestData(t, ctx, testclient.Data{})
612+
run := &params.Run{
613+
Clients: clients.Clients{
614+
Kube: stdata.Kube,
615+
Log: log,
616+
},
617+
}
618+
619+
client, mux, tearDown := thelp.Setup(t)
620+
defer tearDown()
621+
622+
v := &Provider{
623+
run: params.New(),
624+
Logger: log,
625+
pacInfo: &info.PacOpts{
626+
Settings: settings.Settings{
627+
ApplicationName: settings.PACApplicationNameDefaultValue,
628+
},
629+
},
630+
eventEmitter: events.NewEventEmitter(run.Clients.Kube, log),
631+
}
632+
v.SetGitLabClient(client)
633+
634+
event := &info.Event{
635+
TriggerTarget: "pull_request",
636+
SourceProjectID: 700,
637+
TargetProjectID: 700,
638+
SHA: "cafe0000",
639+
PullRequestNumber: 1,
640+
}
641+
642+
mux.HandleFunc(fmt.Sprintf("/projects/%d/statuses/%s", event.SourceProjectID, event.SHA),
643+
func(rw http.ResponseWriter, r *http.Request) {
644+
var reqBody map[string]any
645+
if err := json.NewDecoder(r.Body).Decode(&reqBody); err != nil {
646+
t.Fatalf("failed to decode request body: %v", err)
647+
}
648+
// pipeline_id should never be sent since response always returns 0
649+
_, hasPipelineID := reqBody["pipeline_id"]
650+
assert.Assert(t, !hasPipelineID,
651+
"pipeline_id should not be sent when cached value is 0")
652+
rw.WriteHeader(http.StatusCreated)
653+
// Response with pipeline_id 0 — should not be cached
654+
fmt.Fprint(rw, `{"id": 1, "pipeline_id": 0}`)
655+
})
656+
657+
statusOpts := providerstatus.StatusOpts{
658+
Conclusion: "success",
659+
OriginalPipelineRunName: "pr-1",
660+
}
661+
err := v.CreateStatus(ctx, event, statusOpts)
662+
assert.NilError(t, err)
663+
assert.Assert(t, v.commitStatusPipelineIDs == nil,
664+
"pipeline ID 0 should not be cached")
665+
666+
// Second call — still no pipeline_id
667+
err = v.CreateStatus(ctx, event, statusOpts)
668+
assert.NilError(t, err)
669+
}
670+
671+
func TestCreateStatusPipelineIDSeparateSHAs(t *testing.T) {
672+
ctx, _ := rtesting.SetupFakeContext(t)
673+
log, _ := logger.GetLogger()
674+
stdata, _ := testclient.SeedTestData(t, ctx, testclient.Data{})
675+
run := &params.Run{
676+
Clients: clients.Clients{
677+
Kube: stdata.Kube,
678+
Log: log,
679+
},
680+
}
681+
682+
client, mux, tearDown := thelp.Setup(t)
683+
defer tearDown()
684+
685+
v := &Provider{
686+
run: params.New(),
687+
Logger: log,
688+
pacInfo: &info.PacOpts{
689+
Settings: settings.Settings{
690+
ApplicationName: settings.PACApplicationNameDefaultValue,
691+
},
692+
},
693+
eventEmitter: events.NewEventEmitter(run.Clients.Kube, log),
694+
}
695+
v.SetGitLabClient(client)
696+
697+
sha1 := "sha1aaaa"
698+
sha2 := "sha2bbbb"
699+
projectID := int64(800)
700+
701+
mux.HandleFunc(fmt.Sprintf("/projects/%d/statuses/%s", projectID, sha1),
702+
func(rw http.ResponseWriter, _ *http.Request) {
703+
rw.WriteHeader(http.StatusCreated)
704+
fmt.Fprint(rw, `{"id": 1, "pipeline_id": 1111}`)
705+
})
706+
mux.HandleFunc(fmt.Sprintf("/projects/%d/statuses/%s", projectID, sha2),
707+
func(rw http.ResponseWriter, _ *http.Request) {
708+
rw.WriteHeader(http.StatusCreated)
709+
fmt.Fprint(rw, `{"id": 2, "pipeline_id": 2222}`)
710+
})
711+
712+
event1 := &info.Event{
713+
TriggerTarget: "pull_request",
714+
SourceProjectID: projectID,
715+
TargetProjectID: projectID,
716+
SHA: sha1,
717+
PullRequestNumber: 1,
718+
}
719+
event2 := &info.Event{
720+
TriggerTarget: "pull_request",
721+
SourceProjectID: projectID,
722+
TargetProjectID: projectID,
723+
SHA: sha2,
724+
PullRequestNumber: 2,
725+
}
726+
727+
statusOpts := providerstatus.StatusOpts{
728+
Conclusion: "success",
729+
OriginalPipelineRunName: "pr-1",
730+
}
731+
732+
err := v.CreateStatus(ctx, event1, statusOpts)
733+
assert.NilError(t, err)
734+
err = v.CreateStatus(ctx, event2, statusOpts)
735+
assert.NilError(t, err)
736+
737+
// Each SHA must have its own cached pipeline ID
738+
assert.Equal(t, v.commitStatusPipelineIDs[pipelineIDKey(projectID, sha1)], int64(1111))
739+
assert.Equal(t, v.commitStatusPipelineIDs[pipelineIDKey(projectID, sha2)], int64(2222))
740+
}
741+
444742
func TestGetCommitInfo(t *testing.T) {
445743
tests := []struct {
446744
name string

0 commit comments

Comments
 (0)
⚡