Skip to content

Commit

Permalink
Change Bitbucket PR hook to point the source branch, commit & ref (#3965
Browse files Browse the repository at this point in the history
)

## Description

This is the first fix for: #3932

Change the Pull Request hook parser to return the source commit, branch, and ref instead of the destination. Right now, the workflow pulls the destination configuration and code. It should pull the source configuration and code to verify that the configuration and code work as expected before merging the changes.

In case of the close event, the hook parser returns the destination branch, ref and merge commit. Usually, the contributor automatically deletes the source branch after merging the changes to the destination branch. Using the source values will cause the workflow to fail.

After the changes, Woodpecker will correctly download the workflow from the source branch (Pull Request commit), but it will fail to clone the repository. This issue is related to the commit format returned by the Bitbucket webhook. This inconsistency has already been reported: https://jira.atlassian.com/browse/BCLOUD-21201. The webhook returns a short SHA. The problem is that the `git fetch` command requires the full SHA. 

A workaround for this issue is to use the ref to fetch the code:

```yaml
clone:
  git:
    image: woodpeckerci/plugin-git
    settings:
      ref: ${CI_COMMIT_REF}
```

This is not ideal, because the Pull Request head won't always match the workflow commit, but it solves 80% of the event use cases (e.g. trigger a pull request workflow on change). This workaround won't work when re-running a previous workflow pointing to another commit, it will pull the last commit, not the previous one.

## Solutions

The solution proposed by the community is to retrieve the full SHA from the Bitbucket API using the short one. This solution has drawbacks:
- The Bitbucket API rate limit is 1000 req/h. This solution will reduce the maximum number of workflow runs per hour.
- It requires a braking change in the forges interface because the ´Hook(...)´ method does not have an instance of the HTTP Client. 

We propose to allow the git plugin to fetch the source code from a URL. The Bitbucket returns a link pointing to the commit. 

This proposal only requires a small change to the git plugin:
- Add a new optional parameter (e.g. CommitLink)
- Add a clause to the following conditional: https://github.com/woodpecker-ci/plugin-git/blob/7ac9615f409b539486b8841bd5ef01ae16bbc434/plugin.go#L79C1-L88C3
```go
if p.Pipeline.CommitLink != "" {...}
```
Git commands:
```shell
$ git fetch --no-tags --depth=1 --filter=tree:0 https://bitbucket.org/workspace/repo/commits/692972aabfec
$ git reset --hard -q 692972aabfec # It works with the short SHA
```
Woodpecker will set CommitLink to a blank string for the other forges, but Bitbuckket will use the one returned by the webhook.
  • Loading branch information
j04n-f authored Jul 23, 2024
1 parent 659e925 commit 047eb19
Show file tree
Hide file tree
Showing 4 changed files with 23 additions and 10 deletions.
16 changes: 12 additions & 4 deletions server/forge/bitbucket/convert.go
Original file line number Diff line number Diff line change
Expand Up @@ -168,22 +168,30 @@ func convertPullHook(from *internal.PullRequestHook) *model.Pipeline {
event = model.EventPullClosed
}

return &model.Pipeline{
pipeline := &model.Pipeline{
Event: event,
Commit: from.PullRequest.Dest.Commit.Hash,
Ref: fmt.Sprintf("refs/heads/%s", from.PullRequest.Dest.Branch.Name),
Commit: from.PullRequest.Source.Commit.Hash,
Ref: fmt.Sprintf("refs/heads/%s", from.PullRequest.Source.Branch.Name),
Refspec: fmt.Sprintf("%s:%s",
from.PullRequest.Source.Branch.Name,
from.PullRequest.Dest.Branch.Name,
),
ForgeURL: from.PullRequest.Links.HTML.Href,
Branch: from.PullRequest.Dest.Branch.Name,
Branch: from.PullRequest.Source.Branch.Name,
Message: from.PullRequest.Desc,
Avatar: from.Actor.Links.Avatar.Href,
Author: from.Actor.Login,
Sender: from.Actor.Login,
Timestamp: from.PullRequest.Updated.UTC().Unix(),
}

if from.PullRequest.State == stateClosed {
pipeline.Commit = from.PullRequest.MergeCommit.Hash
pipeline.Ref = fmt.Sprintf("refs/heads/%s", from.PullRequest.Dest.Branch.Name)
pipeline.Branch = from.PullRequest.Dest.Branch.Name
}

return pipeline
}

// convertPushHook is a helper function used to convert a Bitbucket push
Expand Down
7 changes: 4 additions & 3 deletions server/forge/bitbucket/convert_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ func Test_helper(t *testing.T) {
hook.PullRequest.Dest.Repo.Links.HTML.Href = "https://bitbucket.org/foo/bar"
hook.PullRequest.Source.Branch.Name = "change"
hook.PullRequest.Source.Repo.FullName = "baz/bar"
hook.PullRequest.Source.Commit.Hash = "c8411d7"
hook.PullRequest.Links.HTML.Href = "https://bitbucket.org/foo/bar/pulls/5"
hook.PullRequest.Desc = "updated README"
hook.PullRequest.Updated = time.Now()
Expand All @@ -137,10 +138,10 @@ func Test_helper(t *testing.T) {
g.Assert(pipeline.Event).Equal(model.EventPull)
g.Assert(pipeline.Author).Equal(hook.Actor.Login)
g.Assert(pipeline.Avatar).Equal(hook.Actor.Links.Avatar.Href)
g.Assert(pipeline.Commit).Equal(hook.PullRequest.Dest.Commit.Hash)
g.Assert(pipeline.Branch).Equal(hook.PullRequest.Dest.Branch.Name)
g.Assert(pipeline.Commit).Equal(hook.PullRequest.Source.Commit.Hash)
g.Assert(pipeline.Branch).Equal(hook.PullRequest.Source.Branch.Name)
g.Assert(pipeline.ForgeURL).Equal(hook.PullRequest.Links.HTML.Href)
g.Assert(pipeline.Ref).Equal("refs/heads/main")
g.Assert(pipeline.Ref).Equal("refs/heads/change")
g.Assert(pipeline.Refspec).Equal("change:main")
g.Assert(pipeline.Message).Equal(hook.PullRequest.Desc)
g.Assert(pipeline.Timestamp).Equal(hook.PullRequest.Updated.Unix())
Expand Down
4 changes: 4 additions & 0 deletions server/forge/bitbucket/internal/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,10 @@ type PullRequestHook struct {
Created time.Time `json:"created_on"`
Updated time.Time `json:"updated_on"`

MergeCommit struct {
Hash string `json:"hash"`
} `json:"merge_commit"`

Source struct {
Repo Repo `json:"repository"`
Commit struct {
Expand Down
6 changes: 3 additions & 3 deletions server/forge/bitbucket/parse_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ func Test_parser(t *testing.T) {
g.Assert(err).IsNil()
g.Assert(r.FullName).Equal("user_name/repo_name")
g.Assert(b.Event).Equal(model.EventPull)
g.Assert(b.Commit).Equal("ce5965ddd289")
g.Assert(b.Commit).Equal("d3022fc0ca3d")
})

g.It("should return pull-request details for a pull-request merged payload", func() {
Expand All @@ -76,7 +76,7 @@ func Test_parser(t *testing.T) {
g.Assert(err).IsNil()
g.Assert(r.FullName).Equal("anbraten/test-2")
g.Assert(b.Event).Equal(model.EventPullClosed)
g.Assert(b.Commit).Equal("6c5f0bc9b2aa")
g.Assert(b.Commit).Equal("006704dbeab2")
})

g.It("should return pull-request details for a pull-request closed payload", func() {
Expand All @@ -89,7 +89,7 @@ func Test_parser(t *testing.T) {
g.Assert(err).IsNil()
g.Assert(r.FullName).Equal("anbraten/test-2")
g.Assert(b.Event).Equal(model.EventPullClosed)
g.Assert(b.Commit).Equal("006704dbeab2")
g.Assert(b.Commit).Equal("f90e18fc9d45")
})
})

Expand Down

0 comments on commit 047eb19

Please sign in to comment.