feat(bitbucket): query commit statuses for /retest - #2879
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2879 +/- ##
==========================================
+ Coverage 69.03% 69.04% +0.01%
==========================================
Files 198 198
Lines 16943 16988 +45
==========================================
+ Hits 11696 11730 +34
- Misses 4389 4398 +9
- Partials 858 860 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Paco Review 🔍This PR implements GetCommitStatuses for the Bitbucket Cloud provider (fetching and deduplicating commit statuses from the Bitbucket API) and updates pkg/matcher's filterSuccessfulTemplates to use those statuses, adding a fallback matching path that recomputes the Bitbucket-Cloud-specific truncated status key to catch cases where parseOriginalPRName can't recover the original PipelineRun name. It also adds unit tests for the new GetCommitStatuses implementation, a new test mux helper, a new Statuses type, and a build-tag-gated e2e test validating that /retest only reruns failed pipelines after PipelineRuns are pruned. Review difficulty: 4/5 (Hard) — The change alters cross-provider PipelineRun skip/run decision logic and adds a new provider API integration, so a subtle mistake has a wide blast radius across all providers even though the diff itself is moderate in size. 2 new inline comment(s) found. Reviewed commit: 014563c |
![pipelines-as-code[bot]](https://avatars.githubusercontent.com/in/112592?s=60&v=4){kind=small}
| if cs.Info.Pac != nil { | ||
| pacOpts = cs.Info.GetPacOpts() | ||
| } | ||
| successfulStatusKeys := map[string]struct{}{} |
There was a problem hiding this comment.
[LOW] The new dedup/fallback matching logic (successfulStatusKeys, parseOriginalPRName combined with GetBBCloudStatusKey) is a meaningful behavioral change to filterSuccessfulTemplates, but the only test added in this diff is the //go:build e2e test requiring live Bitbucket credentials, which won't run in the normal unit test suite. A unit test in pkg/matcher covering the new fallback-matching and nil-Pac-guard paths would catch regressions without requiring live infrastructure.
12189c3 to
1f59efe
Compare
|
why [DNM]? |
1f59efe to
ab80c06
Compare
theakshaypant
changed the title
Wanted to avoid double review effort since the implementation is similar to the one for github. I have incorporated the changes requested there. |
ab80c06 to
60d1bb6
Compare
There was a problem hiding this comment.
@zakisk IIRC you had implemented the GetBBCloudStatusKey logic originally; I did a couple of AI reviews and the e2e test helps but would be nice if you can have a deeper look at the changes in this file.
60d1bb6 to
a25651a
Compare
|
/retest |
| assert.Equal(t, len(got), len(tt.wantStatuses)) | ||
| assert.Equal(t, len(tt.wantStatuses), len(got)) | ||
| for i, want := range tt.wantStatuses { | ||
| assert.Equal(t, got[i].Name, want.Name) | ||
| assert.Equal(t, got[i].Status, want.Status) | ||
| assert.Equal(t, want.Name, got[i].Name) | ||
| assert.Equal(t, want.Status, got[i].Status) |
There was a problem hiding this comment.
We have a standard format mentioned here
assert.Equal(t, expected, actual)
I have been messing up the order in the last few PRs, so I was extra cautious this time :P
| name: "Non BB Cloud provider skips GetBBCloudStatusKey fallback", | ||
| providerName: "github", | ||
| commitStatuses: []provider.CommitStatusInfo{ | ||
| {Name: "this-is-a-long-pipelinerun-name-t-989318", Status: "successful"}, |
There was a problem hiding this comment.
this API response should also return failed one "this-is-a-long-pipelinerun-name-that-exceeds-forty-characters" as failed one otherwise it is being added because it wasn't successfulTemplate array
| "github.com/openshift-pipelines/pipelines-as-code/pkg/params/info" | ||
| "github.com/openshift-pipelines/pipelines-as-code/pkg/params/triggertype" | ||
| "github.com/openshift-pipelines/pipelines-as-code/pkg/provider" | ||
| providerstatus "github.com/openshift-pipelines/pipelines-as-code/pkg/provider/status" |
There was a problem hiding this comment.
| providerstatus "github.com/openshift-pipelines/pipelines-as-code/pkg/provider/status" | |
| providerstatus "github.com/openshift-pipelines/pipelines-as-code/pkg/provider/status" | |
| "github.com/openshift-pipelines/pipelines-as-code/pkg/params/info" |
| var pacOpts info.PacOpts | ||
| if cs.Info.Pac != nil { | ||
| pacOpts = cs.Info.GetPacOpts() | ||
| } |
There was a problem hiding this comment.
| } | |
| if cs.Info.Pac != nil { | |
| pacOpts = cs.Info.GetPacOpts() | |
| } else { | |
| pacOpts = info.NewPacOpts() | |
| } |
There was a problem hiding this comment.
you can also add new unit test cases for nil pacinfo with default value of ApplicationName
| if tt.noClient { | ||
| v := &Provider{} | ||
| _, err := v.GetCommitStatuses(ctx, bbcloudtest.MakeEvent(nil)) | ||
| assert.Assert(t, err != nil) | ||
| return | ||
| } |
There was a problem hiding this comment.
| if tt.noClient { | |
| v := &Provider{} | |
| _, err := v.GetCommitStatuses(ctx, bbcloudtest.MakeEvent(nil)) | |
| assert.Assert(t, err != nil) | |
| return | |
| } | |
| v := &Provider{} |
| event := bbcloudtest.MakeEvent(nil) | ||
| bbcloudtest.MuxListCommitStatuses(t, mux, event, tt.statuses) | ||
|
|
||
| v := &Provider{bbClient: bbclient} |
There was a problem hiding this comment.
| v := &Provider{bbClient: bbclient} | |
| if !tt.noClient { | |
| v.bbClient = bbClient | |
| } |
Implement GetCommitStatuses for Bitbucket Cloud so /retest skips pipelines that already succeeded, even when PipelineRun objects have been pruned. The provider returns raw status keys; the annotation matcher uses GetBBCloudStatusKey to reverse-match truncated keys against template names. Signed-off-by: Akshay Pant <akpant@redhat.com>
Swap assert.Equal arguments in TestGetCommitStatuses to follow the project convention of expected before actual. Signed-off-by: Akshay Pant <akpant@redhat.com>
a25651a to
fc935e9
Compare
3 participants
📝 Description of the Change
Implement GetCommitStatuses for Bitbucket Cloud so /retest skips pipelines that already succeeded, even when PipelineRun objects have been pruned. The provider returns raw status keys; the annotation matcher uses GetBBCloudStatusKey to reverse-match truncated keys against template names.
🔗 Linked GitHub Issue
Partially fixes #2580
🧪 Testing Strategy
🤖 AI Assistance
AI assistance can be used for various tasks, such as code generation,
documentation, or testing.
Please indicate whether you have used AI assistance
for this PR and provide details if applicable.
Important
Slop will be simply rejected, if you are using AI assistance you need to make sure you
understand the code generated and that it meets the project's standards. you
need at least know how to run the code and deploy it (if needed). See
startpaac to make it easy
to deploy and test your code changes.
If the majority of the code in this PR was generated by an AI, please add a
Co-authored-bytrailer to your commit message.For example:
Co-authored-by: Claude noreply@anthropic.com
✅ Submitter Checklist
fix:,feat:) matches the "Type of Change" I selected above.make testandmake lintlocally to check for and fix anyissues. For an efficient workflow, I have considered installing
pre-commit and running
pre-commit installtoautomate these checks.