Skip to content

Commit 99dc953

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 99dc953

12 files changed

Lines changed: 524 additions & 45 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: 86 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -745,10 +745,33 @@ func TestGetFiles(t *testing.T) {
745745
},
746746
}
747747

748+
mergeCommitPushEvent := &info.Event{
749+
SHA: "MERGESHA456",
750+
Organization: "pac",
751+
Repository: "test",
752+
TriggerTarget: triggertype.Push,
753+
}
754+
755+
mergeCommitFiles := []*bbtest.DiffStat{
756+
{
757+
Path: bbtest.DiffPath{ToString: "merge-added.go"},
758+
Type: "ADD",
759+
},
760+
{
761+
Path: bbtest.DiffPath{ToString: "merge-modified.txt"},
762+
Type: "MODIFY",
763+
},
764+
{
765+
Path: bbtest.DiffPath{ToString: "merge-deleted.md"},
766+
Type: "DELETE",
767+
},
768+
}
769+
748770
tests := []struct {
749771
name string
750772
event *info.Event
751773
changeFiles []*bbtest.DiffStat
774+
previousHeadCommit string
752775
wantAddedFilesCount int
753776
wantDeletedFilesCount int
754777
wantModifiedFilesCount int
@@ -794,6 +817,23 @@ func TestGetFiles(t *testing.T) {
794817
wantError: true,
795818
errMsg: "failed to list changes for pull request: No message available",
796819
},
820+
{
821+
name: "good/merge commit push event",
822+
event: mergeCommitPushEvent,
823+
changeFiles: mergeCommitFiles,
824+
previousHeadCommit: "PREVIOUSHEAD789",
825+
wantAddedFilesCount: 1,
826+
wantDeletedFilesCount: 1,
827+
wantModifiedFilesCount: 1,
828+
wantRenamedFilesCount: 0,
829+
},
830+
{
831+
name: "bad/merge commit push event api error",
832+
event: mergeCommitPushEvent,
833+
previousHeadCommit: "PREVIOUSHEAD789",
834+
wantError: true,
835+
errMsg: "failed to list changes for commit MERGESHA456: failed to get merge commit changes: status code: 401",
836+
},
797837
}
798838
for _, tt := range tests {
799839
t.Run(tt.name, func(t *testing.T) {
@@ -802,10 +842,24 @@ func TestGetFiles(t *testing.T) {
802842
defer tearDown()
803843

804844
stats := &bbtest.DiffStats{
845+
Pagination: bbtest.Pagination{
846+
LastPage: true,
847+
},
805848
Values: tt.changeFiles,
806849
}
807850

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

834-
v := &Provider{client: client, baseURL: tURL, triggerEvent: string(tt.event.TriggerTarget)}
888+
v := &Provider{client: client, baseURL: tURL, triggerEvent: string(tt.event.TriggerTarget), previousHeadCommit: tt.previousHeadCommit}
835889
changedFiles, err := v.GetFiles(ctx, tt.event)
836890
if tt.wantError {
837891
assert.Equal(t, err.Error(), tt.errMsg)
@@ -855,33 +909,37 @@ func TestGetFiles(t *testing.T) {
855909
}
856910
}
857911

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
912+
// getMergeCommitChanges uses v.client.Do directly (not v.Client()),
913+
// so no API usage metrics are recorded for that path.
914+
if tt.previousHeadCommit == "" {
915+
var rm metricdata.ResourceMetrics
916+
err = reader.Collect(ctx, &rm)
917+
assert.NilError(t, err, "error collecting metrics")
918+
919+
assert.Equal(t, len(rm.ScopeMetrics), 1)
920+
assert.Equal(t, len(rm.ScopeMetrics[0].Metrics), 1)
921+
assert.Equal(t, rm.ScopeMetrics[0].Metrics[0].Name, "pipelines_as_code_git_provider_api_request_count")
922+
count, ok := rm.ScopeMetrics[0].Metrics[0].Data.(metricdata.Sum[int64])
923+
assert.Assert(t, ok)
884924
assert.Equal(t, count.DataPoints[0].Value, int64(1))
925+
926+
_, _ = v.GetFiles(ctx, tt.event)
927+
// recollect the metrics af
928+
err = reader.Collect(ctx, &rm)
929+
assert.NilError(t, err, "error collecting metrics")
930+
931+
assert.Equal(t, len(rm.ScopeMetrics), 1)
932+
assert.Equal(t, len(rm.ScopeMetrics[0].Metrics), 1)
933+
assert.Equal(t, rm.ScopeMetrics[0].Metrics[0].Name, "pipelines_as_code_git_provider_api_request_count")
934+
count, ok = rm.ScopeMetrics[0].Metrics[0].Data.(metricdata.Sum[int64])
935+
assert.Assert(t, ok)
936+
if tt.wantError {
937+
// no caching on error so we expect 2 metrics
938+
assert.Equal(t, count.DataPoints[0].Value, int64(2))
939+
} else {
940+
// caching on success so we expect 1 metric
941+
assert.Equal(t, count.DataPoints[0].Value, int64(1))
942+
}
885943
}
886944
})
887945
}
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)