Skip to content

Commit e20f740

Browse files
zakiskclaude
andcommitted
fix(bitbucket-datacenter): detect changes on merged PR push
When a pull request is merged in Bitbucket Data Center, the resulting push event contains a merge commit that reports no file changes. This caused on-path-change and on-cel-expression filters to silently skip PipelineRuns because the changed files list was always empty. The fix detects merge commits (commits with multiple parents) during payload parsing and uses the Bitbucket /changes API with since/until params to diff the previous HEAD against the current HEAD, retrieving the actual files modified by the merged PR. Key changes: - Parse merge commits in push payloads to capture previousHeadCommit - Add getMergeCommitChanges using /changes API with proper pagination - Add DiffStats/Pagination types for the /changes API response - Update TearDown to skip deletion of already-merged PRs - Allow CreatePR to target a custom base branch for merge test scenarios - Add E2E test for on-path-change annotation surviving a PR merge push - Add unit tests for getMergeCommitChanges and merge commit GetFiles path Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 861a507 commit e20f740

13 files changed

Lines changed: 538 additions & 80 deletions

File tree

go.mod

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ require (
1313
github.com/google/cel-go v0.28.0
1414
github.com/google/go-cmp v0.7.0
1515
github.com/google/go-github/scrape v0.0.0-20260403152401-96a365122246
16+
github.com/google/go-github/v84 v84.0.0
1617
github.com/google/go-github/v85 v85.0.0
1718
github.com/hako/durafmt v0.0.0-20210608085754-5c1018a4e16b
1819
github.com/jenkins-x/go-scm v1.15.17
@@ -78,7 +79,6 @@ require (
7879
github.com/go-openapi/swag/typeutils v0.25.5 // indirect
7980
github.com/go-openapi/swag/yamlutils v0.25.5 // indirect
8081
github.com/go-viper/mapstructure/v2 v2.5.0 // indirect
81-
github.com/google/go-github/v84 v84.0.0 // indirect
8282
github.com/oklog/ulid/v2 v2.1.1 // indirect
8383
github.com/prometheus/otlptranslator v1.0.0 // indirect
8484
github.com/rickb777/plural v1.4.10 // indirect

pkg/provider/bitbucketdatacenter/bitbucketdatacenter.go

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ type Provider struct {
4141
apiURL string
4242
provenance string
4343
projectKey string
44+
previousHeadCommit string
4445
repo *v1alpha1.Repository
4546
triggerEvent string
4647
cachedChangedFiles *changedfiles.ChangedFiles
@@ -422,7 +423,13 @@ func (v *Provider) fetchChangedFiles(ctx context.Context, runevent *info.Event)
422423
case triggertype.Push:
423424
opts := &scm.ListOptions{Page: 1, Size: apiResponseLimit}
424425
for {
425-
changes, _, err := v.Client().Git.ListChanges(ctx, orgAndRepo, runevent.SHA, opts)
426+
var changes []*scm.Change
427+
var err error
428+
if v.previousHeadCommit != "" {
429+
changes, _, err = v.getMergeCommitChanges(ctx, runevent.Organization, runevent.Repository, v.previousHeadCommit, runevent.SHA, opts)
430+
} else {
431+
changes, _, err = v.Client().Git.ListChanges(ctx, orgAndRepo, runevent.SHA, opts)
432+
}
426433
if err != nil {
427434
return changedfiles.ChangedFiles{}, fmt.Errorf("failed to list changes for commit %s: %w", runevent.SHA, err)
428435
}

pkg/provider/bitbucketdatacenter/bitbucketdatacenter_test.go

Lines changed: 99 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import (
2121
"github.com/openshift-pipelines/pipelines-as-code/pkg/params/settings"
2222
"github.com/openshift-pipelines/pipelines-as-code/pkg/params/triggertype"
2323
bbtest "github.com/openshift-pipelines/pipelines-as-code/pkg/provider/bitbucketdatacenter/test"
24+
bbtypes "github.com/openshift-pipelines/pipelines-as-code/pkg/provider/bitbucketdatacenter/types"
2425
"github.com/openshift-pipelines/pipelines-as-code/pkg/provider/status"
2526
"go.opentelemetry.io/otel"
2627

@@ -707,48 +708,71 @@ func TestGetFiles(t *testing.T) {
707708
PullRequestNumber: 1,
708709
}
709710

710-
pushFiles := []*bbtest.DiffStat{
711+
pushFiles := []*bbtypes.DiffStat{
711712
{
712-
Path: bbtest.DiffPath{ToString: "added.md"},
713+
Path: bbtypes.DiffPath{ToString: "added.md"},
713714
Type: "ADD",
714715
},
715716
{
716-
Path: bbtest.DiffPath{ToString: "modified.txt"},
717+
Path: bbtypes.DiffPath{ToString: "modified.txt"},
717718
Type: "MODIFY",
718719
},
719720
{
720-
Path: bbtest.DiffPath{ToString: "renamed.yaml"},
721+
Path: bbtypes.DiffPath{ToString: "renamed.yaml"},
721722
Type: "MOVE",
722723
},
723724
{
724-
Path: bbtest.DiffPath{ToString: "deleted.go"},
725+
Path: bbtypes.DiffPath{ToString: "deleted.go"},
725726
Type: "DELETE",
726727
},
727728
}
728729

729-
pullRequestFiles := []*bbtest.DiffStat{
730+
pullRequestFiles := []*bbtypes.DiffStat{
730731
{
731-
Path: bbtest.DiffPath{ToString: "added.go"},
732+
Path: bbtypes.DiffPath{ToString: "added.go"},
732733
Type: "ADD",
733734
},
734735
{
735-
Path: bbtest.DiffPath{ToString: "modified.yaml"},
736+
Path: bbtypes.DiffPath{ToString: "modified.yaml"},
736737
Type: "MODIFY",
737738
},
738739
{
739-
Path: bbtest.DiffPath{ToString: "renamed.txt"},
740+
Path: bbtypes.DiffPath{ToString: "renamed.txt"},
740741
Type: "MOVE",
741742
},
742743
{
743-
Path: bbtest.DiffPath{ToString: "deleted.md"},
744+
Path: bbtypes.DiffPath{ToString: "deleted.md"},
745+
Type: "DELETE",
746+
},
747+
}
748+
749+
mergeCommitPushEvent := &info.Event{
750+
SHA: "MERGESHA456",
751+
Organization: "pac",
752+
Repository: "test",
753+
TriggerTarget: triggertype.Push,
754+
}
755+
756+
mergeCommitFiles := []*bbtypes.DiffStat{
757+
{
758+
Path: bbtypes.DiffPath{ToString: "merge-added.go"},
759+
Type: "ADD",
760+
},
761+
{
762+
Path: bbtypes.DiffPath{ToString: "merge-modified.txt"},
763+
Type: "MODIFY",
764+
},
765+
{
766+
Path: bbtypes.DiffPath{ToString: "merge-deleted.md"},
744767
Type: "DELETE",
745768
},
746769
}
747770

748771
tests := []struct {
749772
name string
750773
event *info.Event
751-
changeFiles []*bbtest.DiffStat
774+
changeFiles []*bbtypes.DiffStat
775+
previousHeadCommit string
752776
wantAddedFilesCount int
753777
wantDeletedFilesCount int
754778
wantModifiedFilesCount int
@@ -794,18 +818,49 @@ func TestGetFiles(t *testing.T) {
794818
wantError: true,
795819
errMsg: "failed to list changes for pull request: No message available",
796820
},
821+
{
822+
name: "good/merge commit push event",
823+
event: mergeCommitPushEvent,
824+
changeFiles: mergeCommitFiles,
825+
previousHeadCommit: "PREVIOUSHEAD789",
826+
wantAddedFilesCount: 1,
827+
wantDeletedFilesCount: 1,
828+
wantModifiedFilesCount: 1,
829+
wantRenamedFilesCount: 0,
830+
},
831+
{
832+
name: "bad/merge commit push event api error",
833+
event: mergeCommitPushEvent,
834+
previousHeadCommit: "PREVIOUSHEAD789",
835+
wantError: true,
836+
errMsg: "failed to list changes for commit MERGESHA456: failed to get merge commit changes: status code: 401",
837+
},
797838
}
798839
for _, tt := range tests {
799840
t.Run(tt.name, func(t *testing.T) {
800841
ctx, _ := rtesting.SetupFakeContext(t)
801842
client, mux, tearDown, tURL := bbtest.SetupBBDataCenterClient()
802843
defer tearDown()
803844

804-
stats := &bbtest.DiffStats{
845+
stats := &bbtypes.DiffStats{
846+
Pagination: bbtypes.Pagination{
847+
LastPage: true,
848+
},
805849
Values: tt.changeFiles,
806850
}
807851

808-
if tt.event.TriggerTarget == triggertype.Push {
852+
if tt.event.TriggerTarget == triggertype.Push && tt.previousHeadCommit != "" {
853+
mux.HandleFunc("/projects/pac/repos/test/changes", func(w http.ResponseWriter, r *http.Request) {
854+
if tt.wantError {
855+
w.WriteHeader(http.StatusUnauthorized)
856+
return
857+
}
858+
assert.Equal(t, r.URL.Query().Get("since"), tt.previousHeadCommit)
859+
assert.Equal(t, r.URL.Query().Get("until"), tt.event.SHA)
860+
b, _ := json.Marshal(stats)
861+
fmt.Fprint(w, string(b))
862+
})
863+
} else if tt.event.TriggerTarget == triggertype.Push {
809864
mux.HandleFunc("/projects/pac/repos/test/commits/IAMSHA123/changes", func(w http.ResponseWriter, _ *http.Request) {
810865
if tt.wantError {
811866
w.WriteHeader(http.StatusUnauthorized)
@@ -831,7 +886,7 @@ func TestGetFiles(t *testing.T) {
831886
provider := sdkmetric.NewMeterProvider(sdkmetric.WithReader(reader))
832887
otel.SetMeterProvider(provider)
833888

834-
v := &Provider{client: client, baseURL: tURL, triggerEvent: string(tt.event.TriggerTarget)}
889+
v := &Provider{client: client, baseURL: tURL, triggerEvent: string(tt.event.TriggerTarget), previousHeadCommit: tt.previousHeadCommit}
835890
changedFiles, err := v.GetFiles(ctx, tt.event)
836891
if tt.wantError {
837892
assert.Equal(t, err.Error(), tt.errMsg)
@@ -855,33 +910,37 @@ func TestGetFiles(t *testing.T) {
855910
}
856911
}
857912

858-
var rm metricdata.ResourceMetrics
859-
err = reader.Collect(ctx, &rm)
860-
assert.NilError(t, err, "error collecting metrics")
861-
862-
assert.Equal(t, len(rm.ScopeMetrics), 1)
863-
assert.Equal(t, len(rm.ScopeMetrics[0].Metrics), 1)
864-
assert.Equal(t, rm.ScopeMetrics[0].Metrics[0].Name, "pipelines_as_code_git_provider_api_request_count")
865-
count, ok := rm.ScopeMetrics[0].Metrics[0].Data.(metricdata.Sum[int64])
866-
assert.Assert(t, ok)
867-
assert.Equal(t, count.DataPoints[0].Value, int64(1))
868-
869-
_, _ = v.GetFiles(ctx, tt.event)
870-
// recollect the metrics af
871-
err = reader.Collect(ctx, &rm)
872-
assert.NilError(t, err, "error collecting metrics")
873-
874-
assert.Equal(t, len(rm.ScopeMetrics), 1)
875-
assert.Equal(t, len(rm.ScopeMetrics[0].Metrics), 1)
876-
assert.Equal(t, rm.ScopeMetrics[0].Metrics[0].Name, "pipelines_as_code_git_provider_api_request_count")
877-
count, ok = rm.ScopeMetrics[0].Metrics[0].Data.(metricdata.Sum[int64])
878-
assert.Assert(t, ok)
879-
if tt.wantError {
880-
// no caching on error so we expect 2 metrics
881-
assert.Equal(t, count.DataPoints[0].Value, int64(2))
882-
} else {
883-
// caching on success so we expect 1 metric
913+
// getMergeCommitChanges uses v.client.Do directly (not v.Client()),
914+
// so no API usage metrics are recorded for that path.
915+
if tt.previousHeadCommit == "" {
916+
var rm metricdata.ResourceMetrics
917+
err = reader.Collect(ctx, &rm)
918+
assert.NilError(t, err, "error collecting metrics")
919+
920+
assert.Equal(t, len(rm.ScopeMetrics), 1)
921+
assert.Equal(t, len(rm.ScopeMetrics[0].Metrics), 1)
922+
assert.Equal(t, rm.ScopeMetrics[0].Metrics[0].Name, "pipelines_as_code_git_provider_api_request_count")
923+
count, ok := rm.ScopeMetrics[0].Metrics[0].Data.(metricdata.Sum[int64])
924+
assert.Assert(t, ok)
884925
assert.Equal(t, count.DataPoints[0].Value, int64(1))
926+
927+
_, _ = v.GetFiles(ctx, tt.event)
928+
// recollect the metrics af
929+
err = reader.Collect(ctx, &rm)
930+
assert.NilError(t, err, "error collecting metrics")
931+
932+
assert.Equal(t, len(rm.ScopeMetrics), 1)
933+
assert.Equal(t, len(rm.ScopeMetrics[0].Metrics), 1)
934+
assert.Equal(t, rm.ScopeMetrics[0].Metrics[0].Name, "pipelines_as_code_git_provider_api_request_count")
935+
count, ok = rm.ScopeMetrics[0].Metrics[0].Data.(metricdata.Sum[int64])
936+
assert.Assert(t, ok)
937+
if tt.wantError {
938+
// no caching on error so we expect 2 metrics
939+
assert.Equal(t, count.DataPoints[0].Value, int64(2))
940+
} else {
941+
// caching on success so we expect 1 metric
942+
assert.Equal(t, count.DataPoints[0].Value, int64(1))
943+
}
885944
}
886945
})
887946
}
Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
package bitbucketdatacenter
2+
3+
import (
4+
"context"
5+
"encoding/json"
6+
"fmt"
7+
"net/http"
8+
"net/url"
9+
10+
"github.com/jenkins-x/go-scm/scm"
11+
"github.com/openshift-pipelines/pipelines-as-code/pkg/provider/bitbucketdatacenter/types"
12+
)
13+
14+
// getMergeCommitChanges gets the changes between two commits for a merge commit.
15+
// this endpoint is not implemented in the go-scm package, so we need to use the raw HTTP request.
16+
// but we're using the client from the go-scm package to make the request.
17+
func (v *Provider) getMergeCommitChanges(ctx context.Context, org, repo, fromCommit, toCommit string, opts *scm.ListOptions) ([]*scm.Change, *scm.Response, error) {
18+
params := url.Values{}
19+
params.Set("since", fromCommit)
20+
params.Set("until", toCommit)
21+
params.Set("start", fmt.Sprintf("%d", (opts.Page-1)*opts.Size))
22+
params.Set("limit", fmt.Sprintf("%d", opts.Size))
23+
path := fmt.Sprintf("rest/api/1.0/projects/%s/repos/%s/changes?%s",
24+
url.PathEscape(org),
25+
url.PathEscape(repo),
26+
params.Encode())
27+
28+
resp, err := v.Client().Do(ctx, &scm.Request{
29+
Method: "GET",
30+
Path: path,
31+
Header: http.Header{
32+
"Content-Type": {"application/json"},
33+
"x-atlassian-token": {"no-check"},
34+
},
35+
})
36+
if err != nil {
37+
return nil, nil, fmt.Errorf("failed to get merge commit changes: %w", err)
38+
}
39+
defer resp.Body.Close()
40+
41+
if resp.Status > 300 {
42+
var errResp struct {
43+
Errors []struct {
44+
Message string `json:"message"`
45+
} `json:"errors"`
46+
}
47+
errMsg := fmt.Sprintf("status code: %d", resp.Status)
48+
if decErr := json.NewDecoder(resp.Body).Decode(&errResp); decErr == nil && len(errResp.Errors) > 0 {
49+
errMsg = fmt.Sprintf("%s, message: %s", errMsg, errResp.Errors[0].Message)
50+
}
51+
return nil, nil, fmt.Errorf("failed to get merge commit changes: %s", errMsg)
52+
}
53+
54+
out := new(types.DiffStats)
55+
56+
err = json.NewDecoder(resp.Body).Decode(out)
57+
if err != nil {
58+
return nil, nil, fmt.Errorf("failed to decode merge commit changes: %w", err)
59+
}
60+
61+
if !out.LastPage {
62+
resp.Page.First = 1
63+
resp.Page.Next = opts.Page + 1
64+
}
65+
66+
return convertDiffstats(out), resp, nil
67+
}
68+
69+
func convertDiffstats(from *types.DiffStats) []*scm.Change {
70+
to := make([]*scm.Change, 0, len(from.Values))
71+
for _, v := range from.Values {
72+
to = append(to, convertDiffstat(v))
73+
}
74+
return to
75+
}
76+
77+
func convertDiffstat(from *types.DiffStat) *scm.Change {
78+
to := &scm.Change{
79+
Path: from.Path.ToString,
80+
Added: from.Type == "ADD",
81+
Modified: from.Type == "MODIFY",
82+
Renamed: from.Type == "MOVE",
83+
Deleted: from.Type == "DELETE",
84+
}
85+
if from.SrcPath != nil {
86+
to.PreviousPath = from.SrcPath.ToString
87+
}
88+
return to
89+
}

0 commit comments

Comments
 (0)