Implement parsing/handling for missing pull request webhook events for BitBucket Server (Stash) driver #130

raphendyr · 2021-11-23T14:21:41Z

I looked into getting PR events working with the Stash driver to support my teams current workflow. My goal was to address issues mentioned in #116, but I did find few other missing events too.

I open this PR for comments. I have tested these changes with a single user and a single repo in my local BitBucket server, so I'm not yet sure if a PR from a fork work as expected. In addition, I have not written any tests yet.

I'm particularly looking feedback on the parts of the code, which I have added GitHub comments for.

Fixes #116

CLAassistant · 2021-11-23T14:21:54Z

All committers have signed the CLA.

raphendyr · 2021-11-23T14:23:37Z

scm/driver/stash/pr.go

+ Base: scm.Reference{
+ Name: from.ToRef.DisplayID,
+ Path: from.ToRef.ID,
+ Sha: from.ToRef.LatestCommit,


I think these should be correct. However, I didn't find use for anything else except .Base.Sha in Drone server

https://github.com/harness/drone/blob/master/service/hook/parser/parse.go#L275

So, should this addition be omitted from the PR?

raphendyr · 2021-11-23T14:24:39Z

scm/driver/stash/webhook.go

+ case "pr:from_ref_updated":
+ // The request includes field "previousFromHash", which could be compared
+ // to the FromRef.Latestcommit to ensure there is actually a change,
+ // but there is unlikely need for that.


Should these comments be added to the git commit instead? They would be available via e.g. blame feature in Github.

raphendyr · 2021-11-23T14:26:22Z

scm/driver/stash/webhook.go

+ ID string `json:"id"` // "refs\/heads\/master"
+ DisplayID string `json:"displayId"` // "master"
+ Type string `json:"type"` // "BRANCH"
+ LatestCommit string `json:"latestCommit"` // "860c4eb4ed0f969b47144234ba13c31c498cca69"


Only fields . PreviousTarget and .PreviousTarget.LatestCommit are currently used. Should I remove everything else from here?

Additionally, if not, should I keep the examples of the data in comments or should I drop those?

bradrydzewski · 2021-11-23T14:39:58Z

👋 hey there, I think in order to provide a review we would need unit tests. This is largely a data mapping exercise and the unit tests include sample webhooks (with real world data) before and after parsing, which are critical to the review process.

bradrydzewski · 2021-11-23T14:48:19Z

scm/driver/stash/webhook.go

+ // action only when the target reference has changed (name or hash).
+ if src.PullRequest.ToRef.DisplayID == src.PreviousTarget.DisplayID &&
+ src.PullRequest.ToRef.LatestCommit == src.PreviousTarget.LatestCommit {
+ return nil, nil


I presume this is checking to differentiate between title / description change vs a code change? If this is just a title or description change we can set the action to Updated as opposed to returning a nil.

if src.PullRequest.ToRef.DisplayID == src.PreviousTarget.DisplayID && src.PullRequest.ToRef.LatestCommit == src.PreviousTarget.LatestCommit { + dst.Action = scm.ActionUpdate } else { dst.Action = scm.ActionSync }

It is. I'll make that change. I wasn't sure what the ActionUpdate was used for.

Although, the same event can include updates to the title/description and changes to the target branch. Should that be mentioned here?

raphendyr · 2021-11-23T14:50:17Z

@bradrydzewski Sure, I was planning to do that next, likely tomorrow. I just wanted to publish what I have done so far for others to read or comment on the things which are not specific to the implementation (i.e., what to include or where to put comments).

raphendyr · 2021-11-24T15:08:50Z

I added some test data for the events. I copied the json data from bitbucket server, but I replaced author info to match what was used before.

I noticed that the newer data includes links to different things. So, I think these TODOs might be fixed?

webhook.go:https:// TODO(bradrydzewski) pr hook does not include repository git+http link
webhook.go:https:// TODO(bradrydzewski) pr hook does not include repository git+ssh link
webhook.go:https:// TODO(bradrydzewski) pr hook does not include repository html link

I can add updated pr_open.json, which includes links. I saved a json data from creating a PR from a fork, so I can use that or one without the fork info. Or I can add that as another test, thus yielding one pr:open test for in repo PR and test for a PR from a fork. The pr_modified_meta.json was created from the PR from the fork, so it currently includes the different kind of data structure.

raphendyr · 2021-11-24T15:30:54Z

Also test coverage seems to be a bit lower for Stash than others 🤔

ok github.com/drone/go-scm/scm/driver/stash 0.019s coverage: 89.3% of statements

EDIT: untested lines are errors and unimplemented functions, so all good 🙂

d1wilko · 2021-11-30T10:18:21Z

hey @raphendyr - I see this is still marked as work in progress, please let us know when you would like us to review :)

raphendyr · 2021-11-30T15:34:10Z

hey @raphendyr - I see this is still marked as work in progress, please let us know when you would like us to review :)

Sorry, my head was stuck waiting possible answers to my questions and without answers, I got stuck there.

tphoney · 2022-01-11T13:49:02Z

Hey @raphendyr i have spent some time getting familiar with this PR and the change. Code wise it looks solid and all of the testing is great to have, and made reading through the code much easier.
Going on to your questions
`Only fields . PreviousTarget and .PreviousTarget.LatestCommit are currently used. Should I remove everything else from here?

Additionally, if not, should I keep the examples of the data in comments or should I drop those?`
please keep the fields.

I think these should be correct. However, I didn't find use for anything else except .Base.Sha in Drone server

https://github.com/harness/drone/blob/master/service/hook/parser/parse.go#L275

So, should this addition be omitted from the PR?

please add these. it brings it in line with the other transports.

Should these comments be added to the git commit instead? They would be available via e.g. blame feature in Github.

please leave these in comments in the file, as it aides in reading.

So leave everything as it is, but could you squash the commits.

Many thanks for your patience, we have been busy else where.

TP

raphendyr · 2022-01-11T14:13:21Z

@tphoney Thanks for the feedback. As per your request, I'll squash the commits to a single commit.

* handle rebases to a pull request * handle target branch changes of a pull request * handle deletion of a pull request

tphoney · 2022-01-12T11:56:12Z

Many thanks @raphendyr for your patience and your excellent code. 🍻

raphendyr commented Nov 23, 2021

View reviewed changes

bradrydzewski reviewed Nov 23, 2021

View reviewed changes

raphendyr marked this pull request as ready for review November 30, 2021 15:32

Implement pull-request related events in Stash driver

e4fe588

* handle rebases to a pull request * handle target branch changes of a pull request * handle deletion of a pull request

tphoney added the bug label Jan 12, 2022

tphoney merged commit b926ba7 into drone:master Jan 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement parsing/handling for missing pull request webhook events for BitBucket Server (Stash) driver #130

Implement parsing/handling for missing pull request webhook events for BitBucket Server (Stash) driver #130

raphendyr commented Nov 23, 2021 •

edited

Loading

CLAassistant commented Nov 23, 2021 •

edited

Loading

raphendyr Nov 23, 2021

raphendyr Nov 23, 2021

raphendyr Nov 23, 2021

bradrydzewski commented Nov 23, 2021

bradrydzewski Nov 23, 2021

raphendyr Nov 23, 2021 •

edited

Loading

raphendyr commented Nov 23, 2021

raphendyr commented Nov 24, 2021 •

edited

Loading

raphendyr commented Nov 24, 2021 •

edited

Loading

d1wilko commented Nov 30, 2021

raphendyr commented Nov 30, 2021

tphoney commented Jan 11, 2022

raphendyr commented Jan 11, 2022

tphoney commented Jan 12, 2022

Implement parsing/handling for missing pull request webhook events for BitBucket Server (Stash) driver #130

Implement parsing/handling for missing pull request webhook events for BitBucket Server (Stash) driver #130

Conversation

raphendyr commented Nov 23, 2021 • edited Loading

CLAassistant commented Nov 23, 2021 • edited Loading

raphendyr Nov 23, 2021

Choose a reason for hiding this comment

raphendyr Nov 23, 2021

Choose a reason for hiding this comment

raphendyr Nov 23, 2021

Choose a reason for hiding this comment

bradrydzewski commented Nov 23, 2021

bradrydzewski Nov 23, 2021

Choose a reason for hiding this comment

raphendyr Nov 23, 2021 • edited Loading

Choose a reason for hiding this comment

raphendyr commented Nov 23, 2021

raphendyr commented Nov 24, 2021 • edited Loading

raphendyr commented Nov 24, 2021 • edited Loading

d1wilko commented Nov 30, 2021

raphendyr commented Nov 30, 2021

tphoney commented Jan 11, 2022

raphendyr commented Jan 11, 2022

tphoney commented Jan 12, 2022

raphendyr commented Nov 23, 2021 •

edited

Loading

CLAassistant commented Nov 23, 2021 •

edited

Loading

raphendyr Nov 23, 2021 •

edited

Loading

raphendyr commented Nov 24, 2021 •

edited

Loading

raphendyr commented Nov 24, 2021 •

edited

Loading