Skip to content

Commit d674217

Browse files
chmouelzakisk
authored andcommitted
feat: Implement setting to skip push events for PR commits
* Add `skip-push-event-for-pr-commits` configuration setting. * Skip processing push events when the commit SHA is present in an open pull request. * Prevent duplicate pipeline runs triggered by both push and pull request events for the same commit. * Document the new configuration setting. Fixes #2101 Signed-off-by: Chmouel Boudjnah <[email protected]> Co-authored-by: Gemini
1 parent 0018148 commit d674217

File tree

11 files changed

+553
-19
lines changed

11 files changed

+553
-19
lines changed

config/302-pac-configmap.yaml

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,13 @@ data:
143143
# you may want to disable this if ok-to-test should be done on each iteration
144144
remember-ok-to-test: "false"
145145

146+
# When enabled, this option prevents duplicate pipeline runs when a commit appears in
147+
# both a push event and a pull request. If a push event comes from a commit that is
148+
# part of an open pull request, the push event will be skipped as it would create
149+
# a duplicate pipeline run.
150+
# Default: true
151+
skip-push-event-for-pr-commits: "true"
152+
146153
# Configure a custom console here, the driver support custom parameters from
147154
# Repo CR along a few other template variable, see documentation for more
148155
# details

docs/content/docs/install/settings.md

Lines changed: 27 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,20 @@ There is a few things you can configure through the config map
139139
risk and should be aware of the potential security vulnerabilities.
140140
(only GitHub and Gitea is supported at the moment).
141141

142+
* `skip-push-event-for-pr-commits`
143+
144+
When enabled, this option prevents duplicate pipeline runs when a commit appears in
145+
both a push event and a pull request. If a push event comes from a commit that is
146+
part of an open pull request, the push event will be skipped as it would create
147+
a duplicate pipeline run.
148+
149+
This feature works by checking if a pushed commit SHA exists in any open pull request,
150+
and if so, skipping the push event processing.
151+
152+
Default: `true`
153+
154+
{{< support_matrix github_app="true" github_webhook="true" gitea="false" gitlab="false" bitbucket_cloud="false" bitbucket_datacenter="false" >}}
155+
142156
### Global Cancel In Progress Settings
143157

144158
* `enable-cancel-in-progress-on-pull-requests`
@@ -429,28 +443,28 @@ A few settings are available to configure this feature:
429443
}
430444
```
431445

432-
The `loglevel.*` fields define the log level for the controllers:
446+
The `loglevel.*` fields define the log level for the controllers:
433447

434448
* loglevel.pipelinesascode - the log level for the pipelines-as-code-controller component
435449
* loglevel.pipelines-as-code-webhook - the log level for the pipelines-as-code-webhook component
436450
* loglevel.pac-watcher - the log level for the pipelines-as-code-watcher component
437451

438-
You can change the log level from `info` to `debug` or any other supported values. For example, select the `debug` log level for the pipelines-as-code-watcher component:
452+
You can change the log level from `info` to `debug` or any other supported values. For example, select the `debug` log level for the pipelines-as-code-watcher component:
439453

440-
```bash
441-
kubectl patch configmap pac-config-logging -n pipelines-as-code --type json -p '[{"op": "replace", "path": "/data/loglevel.pac-watcher", "value":"debug"}]'
442-
```
454+
```bash
455+
kubectl patch configmap pac-config-logging -n pipelines-as-code --type json -p '[{"op": "replace", "path": "/data/loglevel.pac-watcher", "value":"debug"}]'
456+
```
443457

444-
After this command, the controller gets a new log level value.
445-
If you want to use the same log level for all Pipelines-as-Code components, delete `level.*` values from configmap:
458+
After this command, the controller gets a new log level value.
459+
If you want to use the same log level for all Pipelines-as-Code components, delete `level.*` values from configmap:
446460

447-
```bash
448-
kubectl patch configmap pac-config-logging -n pipelines-as-code --type json -p '[ {"op": "remove", "path": "/data/loglevel.pac-watcher"}, {"op": "remove", "path": "/data/loglevel.pipelines-as-code-webhook"}, {"op": "remove", "path": "/data/loglevel.pipelinesascode"}]'
449-
```
461+
```bash
462+
kubectl patch configmap pac-config-logging -n pipelines-as-code --type json -p '[ {"op": "remove", "path": "/data/loglevel.pac-watcher"}, {"op": "remove", "path": "/data/loglevel.pipelines-as-code-webhook"}, {"op": "remove", "path": "/data/loglevel.pipelinesascode"}]'
463+
```
450464

451-
In this case, all Pipelines-as-Code components get a common log level from `zap-logger-config` - `level` field from the json.
465+
In this case, all Pipelines-as-Code components get a common log level from `zap-logger-config` - `level` field from the json.
452466

453-
`zap-logger-config` supports the following log levels:
467+
`zap-logger-config` supports the following log levels:
454468

455469
* debug - fine-grained debugging
456470
* info - normal logging
@@ -460,4 +474,4 @@ A few settings are available to configure this feature:
460474
* panic - trigger a panic (crash)
461475
* fatal - immediately exit with exit status 1 (failure)
462476

463-
See more: <https://knative.dev/docs/serving/observability/logging/config-logging>
477+
See more: <https://knative.dev/docs/serving/observability/logging/config-logging>

pkg/params/settings/config.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,8 @@ type Settings struct {
6666
EnableCancelInProgressOnPullRequests bool `json:"enable-cancel-in-progress-on-pull-requests"`
6767
EnableCancelInProgressOnPush bool `json:"enable-cancel-in-progress-on-push"`
6868

69+
SkipPushEventForPRCommits bool `json:"skip-push-event-for-pr-commits" default:"true"` // nolint:tagalign
70+
6971
CustomConsoleName string `json:"custom-console-name"`
7072
CustomConsoleURL string `json:"custom-console-url"`
7173
CustomConsolePRdetail string `json:"custom-console-url-pr-details"`

pkg/params/settings/config_test.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ func TestSyncConfig(t *testing.T) {
4040
ErrorDetectionSimpleRegexp: "^(?P<filename>[^:]*):(?P<line>[0-9]+):(?P<column>[0-9]+)?([ ]*)?(?P<error>.*)",
4141
EnableCancelInProgressOnPullRequests: false,
4242
EnableCancelInProgressOnPush: false,
43+
SkipPushEventForPRCommits: true,
4344
CustomConsoleName: "",
4445
CustomConsoleURL: "",
4546
CustomConsolePRdetail: "",
@@ -73,6 +74,7 @@ func TestSyncConfig(t *testing.T) {
7374
"custom-console-url-pr-tasklog": "https://custom-console-pr-tasklog",
7475
"custom-console-url-namespace": "https://custom-console-namespace",
7576
"remember-ok-to-test": "false",
77+
"skip-push-event-for-pr-commits": "true",
7678
},
7779
expectedStruct: Settings{
7880
ApplicationName: "pac-pac",
@@ -98,6 +100,7 @@ func TestSyncConfig(t *testing.T) {
98100
CustomConsolePRTaskLog: "https://custom-console-pr-tasklog",
99101
CustomConsoleNamespaceURL: "https://custom-console-namespace",
100102
RememberOKToTest: false,
103+
SkipPushEventForPRCommits: true,
101104
},
102105
},
103106
{

pkg/params/settings/convert_test.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ func TestConvert(t *testing.T) {
3535
"error-log-snippet": "true",
3636
"enable-cancel-in-progress-on-pull-requests": "false",
3737
"enable-cancel-in-progress-on-push": "false",
38+
"skip-push-event-for-pr-commits": "true",
3839
"hub-catalog-name": "tekton",
3940
"hub-url": "https://api.hub.tekton.dev/v1",
4041
"max-keep-run-upper-limit": "0",
@@ -75,6 +76,7 @@ func TestConvert(t *testing.T) {
7576
"error-log-snippet": "true",
7677
"enable-cancel-in-progress-on-pull-requests": "false",
7778
"enable-cancel-in-progress-on-push": "false",
79+
"skip-push-event-for-pr-commits": "true",
7880
"hub-catalog-name": "tekton",
7981
"hub-url": "https://api.hub.tekton.dev/v1",
8082
"max-keep-run-upper-limit": "0",
@@ -116,6 +118,7 @@ func TestConvert(t *testing.T) {
116118
"error-log-snippet": "true",
117119
"enable-cancel-in-progress-on-pull-requests": "false",
118120
"enable-cancel-in-progress-on-push": "false",
121+
"skip-push-event-for-pr-commits": "true",
119122
"hub-catalog-name": "test tekton",
120123
"hub-url": "https://api.hub.tekton.dev/v2",
121124
"max-keep-run-upper-limit": "0",
@@ -169,6 +172,7 @@ func TestConvert(t *testing.T) {
169172
"error-log-snippet": "true",
170173
"enable-cancel-in-progress-on-pull-requests": "false",
171174
"enable-cancel-in-progress-on-push": "false",
175+
"skip-push-event-for-pr-commits": "true",
172176
"hub-catalog-name": "test tekton",
173177
"hub-url": "https://api.hub.tekton.dev/v2",
174178
"max-keep-run-upper-limit": "0",

pkg/provider/github/github_test.go

Lines changed: 186 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,8 @@ import (
2424
"github.com/openshift-pipelines/pipelines-as-code/pkg/params"
2525
"github.com/openshift-pipelines/pipelines-as-code/pkg/params/clients"
2626
"github.com/openshift-pipelines/pipelines-as-code/pkg/params/info"
27+
"github.com/openshift-pipelines/pipelines-as-code/pkg/params/settings"
28+
"github.com/openshift-pipelines/pipelines-as-code/pkg/params/triggertype"
2729
testclient "github.com/openshift-pipelines/pipelines-as-code/pkg/test/clients"
2830
ghtesthelper "github.com/openshift-pipelines/pipelines-as-code/pkg/test/github"
2931
"github.com/openshift-pipelines/pipelines-as-code/pkg/test/logger"
@@ -1304,3 +1306,187 @@ func TestCreateComment(t *testing.T) {
13041306
})
13051307
}
13061308
}
1309+
1310+
func TestSkipPushEventForPRCommits(t *testing.T) {
1311+
iid := int64(1234)
1312+
tests := []struct {
1313+
name string
1314+
pacInfoEnabled bool
1315+
pushEvent *github.PushEvent
1316+
mockAPIs map[string]func(rw http.ResponseWriter, r *http.Request)
1317+
isPartOfPR bool
1318+
wantErr bool
1319+
wantErrContains string
1320+
skipWarnLogContains string
1321+
}{
1322+
{
1323+
name: "skip push event when commit is part of an open PR",
1324+
pacInfoEnabled: true,
1325+
pushEvent: &github.PushEvent{
1326+
Repo: &github.PushEventRepository{
1327+
Name: github.Ptr("testRepo"),
1328+
Owner: &github.User{Login: github.Ptr("testOrg")},
1329+
},
1330+
HeadCommit: &github.HeadCommit{
1331+
ID: github.Ptr("abc123"),
1332+
},
1333+
},
1334+
mockAPIs: map[string]func(rw http.ResponseWriter, r *http.Request){
1335+
"/repos/testOrg/testRepo/commits/abc123/pulls": func(rw http.ResponseWriter, r *http.Request) {
1336+
assert.Equal(t, r.Method, http.MethodGet)
1337+
fmt.Fprint(rw, `[{"number": 42, "state": "open"}]`)
1338+
},
1339+
},
1340+
isPartOfPR: true,
1341+
wantErr: true,
1342+
wantErrContains: "commit abc123 is part of pull request #42, skipping push event",
1343+
},
1344+
{
1345+
name: "continue processing push event when commit is not part of PR",
1346+
pacInfoEnabled: true,
1347+
pushEvent: &github.PushEvent{
1348+
Repo: &github.PushEventRepository{
1349+
Name: github.Ptr("testRepo"),
1350+
Owner: &github.User{Login: github.Ptr("testOrg")},
1351+
DefaultBranch: github.Ptr("main"),
1352+
HTMLURL: github.Ptr("https://github.com/testOrg/testRepo"),
1353+
ID: github.Ptr(iid),
1354+
},
1355+
HeadCommit: &github.HeadCommit{
1356+
ID: github.Ptr("abc123"),
1357+
URL: github.Ptr("https://github.com/testOrg/testRepo/commit/abc123"),
1358+
Message: github.Ptr("Test commit message"),
1359+
},
1360+
Ref: github.Ptr("refs/heads/main"),
1361+
Sender: &github.User{Login: github.Ptr("testUser")},
1362+
},
1363+
mockAPIs: map[string]func(rw http.ResponseWriter, r *http.Request){
1364+
"/repos/testOrg/testRepo/pulls": func(rw http.ResponseWriter, r *http.Request) {
1365+
assert.Equal(t, r.Method, http.MethodGet)
1366+
assert.Equal(t, r.URL.Query().Get("state"), "open")
1367+
fmt.Fprint(rw, `[{"number": 42}]`)
1368+
},
1369+
"/repos/testOrg/testRepo/pulls/42/commits": func(rw http.ResponseWriter, r *http.Request) {
1370+
assert.Equal(t, r.Method, http.MethodGet)
1371+
fmt.Fprint(rw, `[{"sha": "def456"}, {"sha": "xyz789"}]`)
1372+
},
1373+
},
1374+
isPartOfPR: false,
1375+
wantErr: false,
1376+
},
1377+
{
1378+
name: "continue when skip feature is disabled",
1379+
pacInfoEnabled: false,
1380+
pushEvent: &github.PushEvent{
1381+
Repo: &github.PushEventRepository{
1382+
Name: github.Ptr("testRepo"),
1383+
Owner: &github.User{Login: github.Ptr("testOrg")},
1384+
DefaultBranch: github.Ptr("main"),
1385+
HTMLURL: github.Ptr("https://github.com/testOrg/testRepo"),
1386+
ID: github.Ptr(iid),
1387+
},
1388+
HeadCommit: &github.HeadCommit{
1389+
ID: github.Ptr("abc123"),
1390+
URL: github.Ptr("https://github.com/testOrg/testRepo/commit/abc123"),
1391+
Message: github.Ptr("Test commit message"),
1392+
},
1393+
Ref: github.Ptr("refs/heads/main"),
1394+
Sender: &github.User{Login: github.Ptr("testUser")},
1395+
},
1396+
isPartOfPR: false, // This should not be checked when feature is disabled
1397+
wantErr: false,
1398+
},
1399+
{
1400+
name: "log warning when API error occurs",
1401+
pacInfoEnabled: true,
1402+
pushEvent: &github.PushEvent{
1403+
Repo: &github.PushEventRepository{
1404+
Name: github.Ptr("testRepo"),
1405+
Owner: &github.User{Login: github.Ptr("testOrg")},
1406+
},
1407+
HeadCommit: &github.HeadCommit{
1408+
ID: github.Ptr("1234"),
1409+
},
1410+
},
1411+
mockAPIs: map[string]func(rw http.ResponseWriter, r *http.Request){
1412+
"/repos/testOrg/testRepo/pulls": func(rw http.ResponseWriter, _ *http.Request) {
1413+
rw.WriteHeader(http.StatusInternalServerError)
1414+
fmt.Fprint(rw, `{"message": "API error"}`)
1415+
},
1416+
},
1417+
isPartOfPR: false,
1418+
wantErr: false,
1419+
skipWarnLogContains: "Error checking if push commit is part of PR",
1420+
},
1421+
}
1422+
1423+
for _, tt := range tests {
1424+
t.Run(tt.name, func(t *testing.T) {
1425+
ctx, _ := rtesting.SetupFakeContext(t)
1426+
fakeclient, mux, _, teardown := ghtesthelper.SetupGH()
1427+
defer teardown()
1428+
1429+
// Register API endpoints
1430+
for pattern, handler := range tt.mockAPIs {
1431+
mux.HandleFunc(pattern, handler)
1432+
}
1433+
1434+
// Create a logger that captures logs
1435+
observer, logs := zapobserver.New(zap.InfoLevel)
1436+
logger := zap.New(observer).Sugar()
1437+
1438+
// Create provider with the test configuration
1439+
provider := &Provider{
1440+
ghClient: fakeclient,
1441+
Logger: logger,
1442+
pacInfo: &info.PacOpts{
1443+
Settings: settings.Settings{
1444+
SkipPushEventForPRCommits: tt.pacInfoEnabled,
1445+
},
1446+
},
1447+
}
1448+
1449+
// Create event with the right trigger type
1450+
event := info.NewEvent()
1451+
event.TriggerTarget = triggertype.Push
1452+
1453+
// Process the event
1454+
result, err := provider.processEvent(ctx, event, tt.pushEvent)
1455+
1456+
// Check errors if expected
1457+
if tt.wantErr {
1458+
assert.Assert(t, err != nil)
1459+
if tt.wantErrContains != "" {
1460+
assert.ErrorContains(t, err, tt.wantErrContains)
1461+
}
1462+
assert.Assert(t, result == nil, "Expected nil result when error occurs")
1463+
return
1464+
}
1465+
1466+
// If no error expected, check the result
1467+
assert.NilError(t, err)
1468+
assert.Assert(t, result != nil, "Expected non-nil result when no error occurs")
1469+
1470+
// Check event fields were properly processed
1471+
if !tt.pacInfoEnabled || !tt.isPartOfPR {
1472+
assert.Equal(t, result.Organization, tt.pushEvent.GetRepo().GetOwner().GetLogin())
1473+
assert.Equal(t, result.Repository, tt.pushEvent.GetRepo().GetName())
1474+
assert.Equal(t, result.SHA, tt.pushEvent.GetHeadCommit().GetID())
1475+
assert.Equal(t, result.Sender, tt.pushEvent.GetSender().GetLogin())
1476+
}
1477+
1478+
// Check for warning logs if applicable
1479+
if tt.skipWarnLogContains != "" {
1480+
// Look for warning logs
1481+
found := false
1482+
for _, logEntry := range logs.All() {
1483+
if strings.Contains(logEntry.Message, tt.skipWarnLogContains) {
1484+
found = true
1485+
break
1486+
}
1487+
}
1488+
assert.Assert(t, found, "Expected warning log containing: %s", tt.skipWarnLogContains)
1489+
}
1490+
})
1491+
}
1492+
}

0 commit comments

Comments
 (0)