webhooks: Support bitbucket server `push` events by ryanslade · Pull Request #45909 · sourcegraph/sourcegraph-public-snapshot

ryanslade · 2022-12-21T15:59:29Z

Handle Bitbucket Server webhook push events.

Closes https://github.com/sourcegraph/sourcegraph/issues/45764

Test plan

Unit tests added and did full end to end testing locally using ngrok.

sourcegraph-bot · 2022-12-21T16:53:25Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff 9239dcd...f12ac94.

Notify	File(s)
@eseliger	internal/extsvc/bitbucketserver/events.go
@sourcegraph/delivery	doc/admin/config/webhooks.md doc/admin/repo/webhooks.md

indradhanush

Left some suggestions, feel free to ignore if you'd prefer otherwise.

Only one out of curiosity question and one point where I think we might be able to avoid bugs in the future.

Overall nothing that blocks this. LGTM!

indradhanush · 2022-12-22T05:26:16Z

+
+	repoName, err := bitbucketServerNameFromEvent(event)
+	if err != nil {
+		return errors.Wrap(err, "handleGitLabWebhook: get name failed")


Also maybe?

Suggested change

return errors.Wrap(err, "handleGitLabWebhook: get name failed")

return errors.Wrap(err, "handleGitLabWebhook: failed to get event name")

indradhanush · 2022-12-22T05:27:51Z

+	if err != nil {
+		// Repo not existing on Sourcegraph is fine
+		if errcode.IsNotFound(err) {
+			g.logger.Warn("BitbucketServer push webhook received for unknown repo", log.String("repo", string(repoName)))


Do we want to add a hint like this for site admins looking at the logs and getting worried?

Suggested change

g.logger.Warn("BitbucketServer push webhook received for unknown repo", log.String("repo", string(repoName)))

g.logger.Warn("BitbucketServer push webhook received for unknown repo. Was the repo deleted?", log.String("repo", string(repoName)))

I think putting this in docs might be better. IMHO this feels like it makes the logs a bit noisier.

indradhanush · 2022-12-22T05:30:22Z

+
+func bitbucketServerNameFromEvent(event *bitbucketserver.PushEvent) (api.RepoName, error) {
+	if event == nil {
+		return "", errors.New("URL for repository not found")


Why URL for repository? Should this be something like "no event found"?

Suggestion in case you want to accept it:

Suggested change

return "", errors.New("URL for repository not found")

return "", errors.New("no event found")

I made it more expicit: nil PushEvent received

indradhanush · 2022-12-22T05:33:59Z

+		return "", errors.New("URL for repository not found")
+	}
+	for _, link := range event.Repository.Links.Clone {
+		// The ssh link is the closest to our repo name


Out of curiosity: What does being close to our repo name mean here? 🤔

The url's in the other fields have different paths that seem to include other metadata. For example, https://bitbucket.sgdev.org/scm/private/test-2020-06-01.git which has the extra scm path section.

indradhanush · 2022-12-22T05:41:01Z

+		if err != nil {
+			return "", errors.Wrap(err, "unable to parse repository URL")
+		}
+		return api.RepoName(parsed.Hostname() + strings.TrimSuffix(parsed.Path, ".git")), nil


This is good timing. I only happened to learn yesterday that parsed.Hostname() may be an empty string if protocol is missing form the string being passed to url.Parse.

See example here: https://go.dev/play/p/nVMuAvo2v4D

Maybe we should check if parsed.Hostname() is an empty string an in that case return an error?

Oh, nice catch. I'll add some extra test cases and make sure we handle that.

While looking into this I realised that we already have a function to do what we want here which will be much more robust so I'm going to submit another PR that uses it for all 3 code hosts:
https://github.com/sourcegraph/sourcegraph/blob/9239dcd9702c73e5b097a6c7962e2bd6152e1934/internal/cloneurls/clone_urls.go#L25

indradhanush · 2022-12-22T05:44:01Z

+}
+
+func TestBitbucketServerNameFromEvent(t *testing.T) {
+	mkEvent := func(name string, href string) *bitbucketserver.PushEvent {


makeEvent? 😛

indradhanush · 2022-12-22T05:44:33Z

 		return "", errors.Wrap(err, "unable to parse repository URL")
 	}
-	return api.RepoName(parsed.Host + parsed.Path), nil
+	return api.RepoName(parsed.Hostname() + parsed.Path), nil


Same comment as above for parsed.Hostname() being empty and other usages in this PR.

…er_handler.go Co-authored-by: Indradhanush Gupta <indradhanush.gupta@gmail.com>

ryanslade added 2 commits December 21, 2022 15:55

Handle Bitbucket Server push events

355ed79

Add test for bitbucketServerNameFromEvent

ddf7d62

ryanslade added the team/repo-management label Dec 21, 2022

cla-bot Bot added the cla-signed label Dec 21, 2022

Update docs

ac818a0

ryanslade changed the title ~~rs/bitbucket server push event~~ webhooks: Support bitbucket server push events Dec 21, 2022

ryanslade added 4 commits December 21, 2022 17:18

Update CHANGELOG

4186643

Tweak docs

92f541d

Close body after read

456f31f

Handle ping event edge case

77e5fcf

ryanslade requested a review from a team December 21, 2022 16:51

ryanslade marked this pull request as ready for review December 21, 2022 16:51

indradhanush approved these changes Dec 22, 2022

View reviewed changes

ryanslade and others added 7 commits December 22, 2022 14:26

Update enterprise/cmd/frontend/internal/repos/webhooks/bitbucket_serv…

f3c23d6

…er_handler.go Co-authored-by: Indradhanush Gupta <indradhanush.gupta@gmail.com>

Update enterprise/cmd/frontend/internal/repos/webhooks/bitbucket_serv…

ae2ea42

…er_handler.go Co-authored-by: Indradhanush Gupta <indradhanush.gupta@gmail.com>

Rename

45fa739

Better error message

324a130

Fix tests

7e2030d

Merge branch 'main' into rs/bitbucket-server-push-event

3158415

Merge branch 'main' into rs/bitbucket-server-push-event

f12ac94

ryanslade merged commit 3666bac into main Dec 22, 2022

ryanslade deleted the rs/bitbucket-server-push-event branch December 22, 2022 14:08

	return errors.Wrap(err, "handleGitLabWebhook: get name failed")
	return errors.Wrap(err, "handleGitLabWebhook: failed to get event name")

	g.logger.Warn("BitbucketServer push webhook received for unknown repo", log.String("repo", string(repoName)))
	g.logger.Warn("BitbucketServer push webhook received for unknown repo. Was the repo deleted?", log.String("repo", string(repoName)))

	return "", errors.New("URL for repository not found")
	return "", errors.New("no event found")

Conversation

ryanslade commented Dec 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test plan

Uh oh!

sourcegraph-bot commented Dec 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

indradhanush left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ryanslade commented Dec 21, 2022 •

edited

Loading

sourcegraph-bot commented Dec 21, 2022 •

edited

Loading