Skip to content

Commit 5720faf

Browse files
committed
fix: use sync.Once around create_comment API call
this commit uses sync.Once to make sure that create_comment is called only once. it stores the sync.Once instance for every unique PR in sync.Map and retrieves and call the API in it. Signed-off-by: Zaki Shaikh <zashaikh@redhat.com>
1 parent 982d3af commit 5720faf

2 files changed

Lines changed: 21 additions & 32 deletions

File tree

.github/workflows/e2e.yaml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -83,8 +83,8 @@ jobs:
8383
TEST_GITHUB_REPO_OWNER_WEBHOOK: openshift-pipelines/pipelines-as-code-e2e-tests-webhook
8484
TEST_GITHUB_SECOND_API_URL: ghe.pipelinesascode.com
8585
TEST_GITHUB_SECOND_EL_URL: http://ghe.paac-127-0-0-1.nip.io
86-
TEST_GITHUB_SECOND_REPO_INSTALLATION_ID: 1
87-
TEST_GITHUB_SECOND_REPO_OWNER_GITHUBAPP: pipelines-as-code/e2e
86+
TEST_GITHUB_SECOND_REPO_INSTALLATION_ID: 8
87+
TEST_GITHUB_SECOND_REPO_OWNER_GITHUBAPP: pac/zaki-test-pac
8888
TEST_GITHUB_SECOND_TOKEN: ${{ secrets.TEST_GITHUB_SECOND_TOKEN }}
8989
TEST_GITHUB_TOKEN: ${{ secrets.GH_APPS_TOKEN }}
9090
TEST_GITLAB_API_URL: https://gitlab.com

pkg/provider/github/github.go

Lines changed: 19 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import (
44
"context"
55
"encoding/base64"
66
"fmt"
7-
"math/rand"
87
"net/http"
98
"net/url"
109
"regexp"
@@ -42,6 +41,8 @@ const (
4241

4342
var _ provider.Interface = (*Provider)(nil)
4443

44+
var syncMap = sync.Map{}
45+
4546
type Provider struct {
4647
ghClient *github.Client
4748
Logger *zap.SugaredLogger
@@ -803,39 +804,27 @@ func (v *Provider) CreateComment(ctx context.Context, event *info.Event, commit,
803804
}
804805
return nil
805806
}
807+
}
806808

807-
// HACK: Workaround for duplicate comment creation issue.
808-
// In E2E tests, we occasionally see two identical comments created on a PR when
809-
// there should only be one. The root cause is unclear, despite only one
810-
// create_comment API call being logged, two comments appear on the PR.
811-
//
812-
// This workaround adds a random sleep (0-500ms) before re-checking for existing
813-
// comments. This reduces the window where parallel processes might both
814-
// see no existing comment and both decide to create one.
815-
//nolint:gosec // No need for crypto/rand here, just reducing timing window
816-
jitter := time.Duration(rand.Intn(500)) * time.Millisecond
817-
timer := time.NewTimer(jitter)
818-
defer timer.Stop()
819-
820-
select {
821-
case <-ctx.Done():
822-
return ctx.Err()
823-
case <-timer.C:
824-
}
825-
826-
// Re-check if a comment exists now
827-
existingComment, err = v.listAndFindComment(ctx, event, updateMarker)
828-
if err != nil {
829-
return err
830-
}
831-
if existingComment != nil {
832-
return nil
809+
var once *sync.Once
810+
var err error
811+
if event.TriggerTarget == triggertype.PullRequest {
812+
key := fmt.Sprintf("%s/%s/%d", event.Organization, event.Repository, event.PullRequestNumber)
813+
value, _ := syncMap.LoadOrStore(key, &sync.Once{})
814+
var ok bool
815+
once, ok = value.(*sync.Once)
816+
if !ok {
817+
return fmt.Errorf("unexpected type in sync map for key %s", key)
833818
}
819+
} else {
820+
once = &sync.Once{}
834821
}
835822

836-
_, _, err := wrapAPI(v, "create_comment", func() (*github.IssueComment, *github.Response, error) {
837-
return v.Client().Issues.CreateComment(ctx, event.Organization, event.Repository, event.PullRequestNumber, &github.IssueComment{
838-
Body: github.Ptr(commit),
823+
once.Do(func() {
824+
_, _, err = wrapAPI(v, "create_comment", func() (*github.IssueComment, *github.Response, error) {
825+
return v.Client().Issues.CreateComment(ctx, event.Organization, event.Repository, event.PullRequestNumber, &github.IssueComment{
826+
Body: github.Ptr(commit),
827+
})
839828
})
840829
})
841830
return err

0 commit comments

Comments
 (0)