Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Bitbucket server/Bitbucket Data Center provider for git commit status #639

Merged
merged 1 commit into from
Nov 21, 2023

Conversation

gdasson
Copy link
Contributor

@gdasson gdasson commented Oct 31, 2023

Atlassian Bitbucket product offers multiple hosting options as documented here

For Flux users that use Bitbucket server or Bitbucket data center solution(mostly enterprises), there is no provider to support posting Git Commit Statuses. This PR provides the git commit status functionality for Bitbucket Server/Data Center.

[ Note: The reason that we can't use existing bitbucket provider and the need for a new bitbucketserver provider stems from the fact that the REST API used by Bitbucket Server/Data Center product is different from Bitbucket Cloud REST API. The existing bitbucket provider can only support Bitbucket cloud. ]

This PR fixes issue: #507

@gdasson gdasson changed the title Add Bitbucket server (a.k.a Bitbucket Data Center) provider for git commit status Add Bitbucket server/Bitbucket Data Center provider for git commit status Oct 31, 2023
@gdasson
Copy link
Contributor Author

gdasson commented Oct 31, 2023

Screenshots attached to show how the Git Commit Statuses will look like on the Bitbucket Server UI once this is implemented.
Screenshot 2023-10-31 at 1 49 57 PM

Screenshot 2023-10-31 at 1 46 14 PM

Copy link
Member

@makkes makkes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a couple of comments. My main concern is the pulling-in of that library.

api/v1beta1/provider_types.go Outdated Show resolved Hide resolved
docs/spec/v1beta2/providers.md Outdated Show resolved Hide resolved
docs/spec/v1beta2/providers.md Outdated Show resolved Hide resolved
docs/spec/v1beta2/providers.md Outdated Show resolved Hide resolved
docs/spec/v1beta2/providers.md Outdated Show resolved Hide resolved
internal/notifier/bitbucketserver.go Outdated Show resolved Hide resolved
internal/notifier/bitbucketserver.go Outdated Show resolved Hide resolved
internal/notifier/bitbucketserver_test.go Outdated Show resolved Hide resolved
internal/notifier/bitbucketserver_test.go Show resolved Hide resolved
internal/notifier/util.go Outdated Show resolved Hide resolved
@gdasson
Copy link
Contributor Author

gdasson commented Nov 3, 2023

@makkes : Thanks for the review and feedback. I have reimplemented the http calls using resty/http library.

@makkes
Copy link
Member

makkes commented Nov 3, 2023

@makkes : Thanks for the review and feedback. I have reimplemented the http calls using resty/http library.

I really don't see the need to pull in any additional library just for a couple of very simple HTTP calls that can just as well be made using net/http.

@gdasson
Copy link
Contributor Author

gdasson commented Nov 5, 2023

@makkes I have reimplemented the HTTP calls using native net/http library/package. Please let me know for any other feedback. Thx.

Copy link
Member

@makkes makkes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks much better now. I added a couple of additional comments.

docs/spec/v1beta1/providers.md Outdated Show resolved Hide resolved
docs/spec/v1beta2/providers.md Outdated Show resolved Hide resolved
docs/spec/v1beta2/providers.md Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
internal/notifier/factory.go Show resolved Hide resolved
internal/notifier/bitbucketserver.go Outdated Show resolved Hide resolved
internal/notifier/bitbucketserver.go Outdated Show resolved Hide resolved
internal/notifier/bitbucketserver.go Outdated Show resolved Hide resolved
internal/notifier/bitbucketserver.go Outdated Show resolved Hide resolved
internal/notifier/bitbucketserver.go Outdated Show resolved Hide resolved
internal/notifier/bitbucketserver.go Outdated Show resolved Hide resolved
internal/notifier/bitbucketserver.go Outdated Show resolved Hide resolved
internal/notifier/bitbucketserver.go Outdated Show resolved Hide resolved
internal/notifier/bitbucketserver.go Outdated Show resolved Hide resolved
internal/notifier/bitbucketserver.go Outdated Show resolved Hide resolved
internal/notifier/bitbucketserver.go Outdated Show resolved Hide resolved
internal/notifier/bitbucketserver.go Outdated Show resolved Hide resolved
internal/notifier/bitbucketserver.go Outdated Show resolved Hide resolved
Copy link
Contributor

@darkowlzz darkowlzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks great to me.
Left a few minor suggestions. I think we'll ready to merge after that.

Also, please rebase the commits and squash them to a few relevant commits if possible.

internal/notifier/bitbucketserver.go Outdated Show resolved Hide resolved
internal/notifier/bitbucketserver_test.go Outdated Show resolved Hide resolved
internal/notifier/bitbucketserver_test.go Show resolved Hide resolved
Copy link
Member

@makkes makkes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some formatting suggestions. Rest lgtm module what darkowlzz asked for.

docs/spec/v1beta2/providers.md Outdated Show resolved Hide resolved
docs/spec/v1beta2/providers.md Outdated Show resolved Hide resolved
@makkes
Copy link
Member

makkes commented Nov 18, 2023

Not sure what happened here but now there's 2 commits and also changes to .github/workflows/ which shouldn't be here. Please rebase on top of main and squash your commits into one.

@gdasson
Copy link
Contributor Author

gdasson commented Nov 18, 2023

Sorry I had messed up my branch a li'l bit. I have reduced the commits by squashing. Does this look fine now? @makkes @darkowlzz

@makkes
Copy link
Member

makkes commented Nov 19, 2023

You still have 3 commits in this PR, two of which are basically the same, plus a merge commit. Please remove the merge commit (rebase instead of merge) and make sure this PR shows a single commit in the "Commits" tab.

Signed-off-by: gdasson <gaurav.dasson@gmail.com>
@gdasson
Copy link
Contributor Author

gdasson commented Nov 21, 2023

@makkes : Sorry, I was being a bit lazy there 😆. Rebased and squashed down to 1 commit now. Please review. Thanks.

@stefanprodan stefanprodan added the area/alerting Alerting related issues and PRs label Nov 21, 2023
Copy link
Member

@makkes makkes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one last bit, then we're done here. Great job!

| ------------------------------------------------| ----------------- |
| [Azure DevOps](#azure-devops) | `azuredevops` |
| [Bitbucket](#bitbucket) | `bitbucket` |
| [BitbucketServer](#bitbucket-serverdata-center) | `bitbucketserver` |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
| [BitbucketServer](#bitbucket-serverdata-center) | `bitbucketserver` |
| [Bitbucket Server/Data Center](#bitbucket-serverdata-center) | `bitbucketserver` |

Copy link
Member

@makkes makkes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, selected the wrong item in the previous review.

@makkes makkes merged commit 9869e39 into fluxcd:main Nov 21, 2023
7 checks passed
@stefanprodan
Copy link
Member

Thanks @gdasson 🏅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/alerting Alerting related issues and PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants