Skip to content

Commit 5b4173f

Browse files
infernus01chmouel
andauthored
fix(github): change skip log level from error to info
When a push event is skipped because the commit is part of an open pull request, PAC was logging this intentional skip at ERROR level. This caused confusion for users and support teams. This PR fixes error to info-level logging for intentional push event skips. Signed-off-by: Shubham Bhardwaj <[email protected]> Co-authored-by: Chmouel Boudjnah <[email protected]>
1 parent 6a0d179 commit 5b4173f

File tree

5 files changed

+34
-12
lines changed

5 files changed

+34
-12
lines changed

pkg/adapter/sinker.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,10 @@ func (s *sinker) processEventPayload(ctx context.Context, request *http.Request)
3232
s.logger.Errorf("failed to parse event: %v", err)
3333
return err
3434
}
35+
// If ParsePayload returned nil event (intentional skip), exit early
36+
if s.event == nil {
37+
return nil
38+
}
3539

3640
// Enhanced structured logging with source repository context for operators
3741
logFields := []interface{}{
@@ -69,6 +73,9 @@ func (s *sinker) processEvent(ctx context.Context, request *http.Request) error
6973
if err := s.processEventPayload(ctx, request); err != nil {
7074
return err
7175
}
76+
if s.event == nil {
77+
return nil
78+
}
7279
}
7380

7481
p := pipelineascode.NewPacs(s.event, s.vcx, s.run, s.pacInfo, s.kint, s.logger, s.globalRepo)

pkg/provider/github/github_test.go

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1518,8 +1518,8 @@ func TestSkipPushEventForPRCommits(t *testing.T) {
15181518
},
15191519
},
15201520
isPartOfPR: true,
1521-
wantErr: true,
1522-
wantErrContains: "commit abc123 is part of pull request #42, skipping push event",
1521+
wantErr: false,
1522+
wantErrContains: "",
15231523
},
15241524
{
15251525
name: "continue processing push event when commit is not part of PR",
@@ -1645,15 +1645,20 @@ func TestSkipPushEventForPRCommits(t *testing.T) {
16451645

16461646
// If no error expected, check the result
16471647
assert.NilError(t, err)
1648+
1649+
// If push event was skipped (commit is part of PR), result should be nil
1650+
if tt.pacInfoEnabled && tt.isPartOfPR {
1651+
assert.Assert(t, result == nil, "Expected nil result when push event is skipped for PR commit")
1652+
return
1653+
}
1654+
16481655
assert.Assert(t, result != nil, "Expected non-nil result when no error occurs")
16491656

16501657
// Check event fields were properly processed
1651-
if !tt.pacInfoEnabled || !tt.isPartOfPR {
1652-
assert.Equal(t, result.Organization, tt.pushEvent.GetRepo().GetOwner().GetLogin())
1653-
assert.Equal(t, result.Repository, tt.pushEvent.GetRepo().GetName())
1654-
assert.Equal(t, result.SHA, tt.pushEvent.GetHeadCommit().GetID())
1655-
assert.Equal(t, result.Sender, tt.pushEvent.GetSender().GetLogin())
1656-
}
1658+
assert.Equal(t, result.Organization, tt.pushEvent.GetRepo().GetOwner().GetLogin())
1659+
assert.Equal(t, result.Repository, tt.pushEvent.GetRepo().GetName())
1660+
assert.Equal(t, result.SHA, tt.pushEvent.GetHeadCommit().GetID())
1661+
assert.Equal(t, result.Sender, tt.pushEvent.GetSender().GetLogin())
16571662

16581663
// Check for warning logs if applicable
16591664
if tt.skipWarnLogContains != "" {

pkg/provider/github/parse_payload.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -178,6 +178,10 @@ func (v *Provider) ParsePayload(ctx context.Context, run *params.Run, request *h
178178
return nil, err
179179
}
180180

181+
if processedEvent == nil {
182+
return nil, nil
183+
}
184+
181185
processedEvent.Event = eventInt
182186
processedEvent.InstallationID = installationIDFrompayload
183187
processedEvent.GHEURL = event.Provider.URL
@@ -359,7 +363,7 @@ func (v *Provider) processEvent(ctx context.Context, event *info.Event, eventInt
359363
// If the commit is part of a PR, skip processing the push event
360364
if isPartOfPR {
361365
v.Logger.Infof("Skipping push event for commit %s as it belongs to pull request #%d", sha, prNumber)
362-
return nil, fmt.Errorf("commit %s is part of pull request #%d, skipping push event", sha, prNumber)
366+
return nil, nil
363367
}
364368
}
365369

pkg/provider/github/parse_payload_test.go

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -923,10 +923,10 @@ func TestParsePayLoad(t *testing.T) {
923923
},
924924
HeadCommit: &github.HeadCommit{ID: github.Ptr("SHAPush")},
925925
},
926-
shaRet: "SHAPush",
926+
shaRet: "",
927927
skipPushEventForPRCommits: true,
928928
muxReplies: map[string]any{"/repos/owner/pushRepo/commits/SHAPush/pulls": sampleGhPRs},
929-
wantErrString: "commit SHAPush is part of pull request #42, skipping push event",
929+
wantErrString: "",
930930
},
931931
{
932932
name: "good/skip tag push event for skip-pr-commits setting",
@@ -1035,6 +1035,12 @@ func TestParsePayLoad(t *testing.T) {
10351035
return
10361036
}
10371037
assert.NilError(t, err)
1038+
// If shaRet is empty, this is a skip case (push event for PR commit)
1039+
// In this case, ret should be nil
1040+
if tt.shaRet == "" {
1041+
assert.Assert(t, ret == nil, "Expected nil result for skipped push event")
1042+
return
1043+
}
10381044
assert.Assert(t, ret != nil)
10391045
assert.Equal(t, tt.shaRet, ret.SHA)
10401046
if tt.eventType == triggertype.PullRequest.String() {

test/github_pullrequest_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -614,7 +614,7 @@ func TestGithubPullandPushMatchTriggerOnlyPull(t *testing.T) {
614614
assert.NilError(t, err)
615615
ctx = info.StoreNS(ctx, globalNs)
616616

617-
reg := regexp.MustCompile(fmt.Sprintf("commit.*is part of pull request #%d.*skipping push event", g.PRNumber))
617+
reg := regexp.MustCompile(fmt.Sprintf("Skipping push event for commit.*as it belongs to pull request #%d", g.PRNumber))
618618
maxLines := int64(100)
619619
err = twait.RegexpMatchingInControllerLog(ctx, g.Cnx, *reg, 20, "controller", &maxLines)
620620
assert.NilError(t, err)

0 commit comments

Comments
 (0)