Adding bitbucket cloud support #479

gcase555 · 2024-05-13T19:32:02Z

What does this change

This is most of the changes for adding bitbucket cloud support. We have implemented the interface and have the ability to automate creating PRs, updating PRs, Closing PRs, Fetching Repos/PRs and Forking Repos.

What issue does it fix

Closes #99

Notes for the reviewer

We are still adding support for handling multiple workspaces and updating the README with a few notes for bitbucket cloud support. My goal here is to get the PR submitted for early feedback and we will update this PR once we add handling multiple workspaces.

…ation for create and get pull requests

WIP: Adding bitbucket support

add BBC fork and update PR support

gcase555 · 2024-05-13T20:10:13Z

examples/go/upgrade-go-version.sh

@@ -2,5 +2,5 @@

 # Title: Upgrade Go version in go modules

-go mod edit -go 1.18
+go mod edit -go 1.19


We changed this for testing, we'll switch it back

ping on this comment

internal/scm/bitbucketcloud/custom_structs.go

lindell

Thanks for the PR 😄

I started looking at it, but it will take some time since I don't have a lot of time currently. But I'm including my first comments. Feel free to take a look at them right away if you want, or wait until all comments are in if you prefer that 🙂

PS. In which detail do you want the reviews right now? I tried to avoid code style, and similar types of comments because of the current PR state you mentioned. But let me know if you prefer those straight away as well.

cmd/platform.go

internal/scm/bitbucketcloud/bitbucket_cloud.go

lindell · 2024-05-14T19:00:26Z

internal/scm/bitbucketcloud/bitbucket_cloud.go

+		Owner:    bbc.workspaces[0],
+		RepoSlug: splitRepoFullName[1],
+	}
+	defaultReviewers, _ := bbc.bbClient.Repositories.Repository.ListDefaultReviewers(repoOptions)


lindell · 2024-05-14T19:05:51Z

internal/scm/bitbucketcloud/bitbucket_cloud.go

+		newPR.Reviewers = append(newPR.Reviewers, reviewer.Uuid)
+	}
+
+	prOptions := &bitbucket.PullRequestsOptions{


Can pull request happen from a fork in bbc? In that case I don't think the owner here is correct?

We need to add a parameter to allow users to specify a bitbucket "project" to fork into before we can specify the owner fully here.

We were hoping to do that as a separate PR and document that as a known limitation but let us know if that is a blocker we should resolve it before we get this change merged.

I'm fine with merging something that does not have all functionality of all other systems 🙂 But please list these, as I will otherwise make comments on things that will not be relevant to you

I made a note in the README template about bitbucket cloud support and the current limitations that should help clarify things.

lindell · 2024-05-14T19:12:30Z

internal/scm/bitbucketcloud/bitbucket_cloud.go

+		project:    splitRepoFullName[0],
+		repoName:   splitRepoFullName[1],
+		branchName: newPR.Head,
+		prProject:  splitRepoFullName[0],


It seems that project/repoName and prProject/prRepoName are used the same here. I guess it comes from other implementations and it might be good to clarify what they are for there.

The first pair are a reference to the project where the PR is place (where is will merge), while the second one is from which repository the change is currently made. If fork is not defined, these will be the same, since the changes will happen the same project. But they could be different, and in that case I think this will not be right.

gcase555 · 2024-05-15T17:09:32Z

PS. In which detail do you want the reviews right now?

The feedback you have given so far has been good, we were looking to answer things that stood out and could potentially be problems or explain why we had to go with app passwords over OAuth access tokens for this first pull-request for accessibility reasons.

internal/scm/bitbucketcloud/bitbucket_cloud.go

cmd/platform.go

internal/scm/bitbucketcloud/bitbucket_cloud.go

lindell · 2024-05-15T18:13:47Z

internal/scm/bitbucketcloud/bitbucket_cloud.go

+		newPR.Reviewers = append(newPR.Reviewers, reviewer.Uuid)
+	}
+
+	prOptions := &bitbucket.PullRequestsOptions{


I'm fine with merging something that does not have all functionality of all other systems 🙂 But please list these, as I will otherwise make comments on things that will not be relevant to you

internal/scm/bitbucketcloud/bitbucket_cloud.go

lindell · 2024-05-20T19:29:42Z

internal/scm/bitbucketcloud/custom_structs.go

+}
+
+// repository contains information about a bitbucket repository
+type repository struct {


Adding workspace name is probably a good idea for later, even if multi workspace support is currently not implemented.

Could this be handled later? I didn't want to refactor too much more and were still only supporting single workspaces(as documented in the README)

Fine for now. Just would make it easier to make those changes later

lindell · 2024-05-20T19:33:38Z

internal/scm/bitbucketcloud/bitbucket_cloud.go

+	return err
+}
+
+func (bbc *BitbucketCloud) GetRepositories(ctx context.Context) ([]scm.Repository, error) {


This seems to only include repository name filtering (--repo), not for example per project which I think is by far the most popular one (organizations for GitHub).

And for specific repo names, it seems that all repositories are fetched. If possible, I think getting those repositories by name should be prefered.

I don't think we had time to implement this change, is it ok if we keep this as is with repository filtering? Trying to avoid heavier refactoring of this PR as it has gotten large and would prefer to do it as a follow up

I'm hesitant to add this as this would heavily limit the expected behaviour. But think it could be ok if

This is clearly described in the Readme (or wherever the info is moved) under the restrictions sections.

Mark it as beta (by adding (beta) to the bitbucket cloud section.

internal/scm/bitbucketcloud/bitbucket_cloud.go

Update Docs and improve PR creation during forks

gcase555 · 2024-06-25T00:44:20Z

@lindell We made as many of the changes as we could with the time we had allotted to work on this and did most of the cleanup and refactoring requested. Please let us know if there are any issues that are blocking and need to be addressed before merging or if there is something we can resolve in a follow up pull request, we are hoping to get this merged as this PR has started getting large and has been outside of master branch for awhile.

I think this implementation covers enough to close out issue #99 and can start being used by end users. We could create a separate issue to handle updates to include project settings and addressing other smaller feedback tasks you feel are non-blocking.

We have successfully started using it to update multiple services across large and small bitbucket cloud workspaces and included documentation on how to get it working and the current limitations.

lindell

As per the pervious comment, I did a pass and tried to be non-nitty.

Just to clarify, you are employees of Atlassian? It does change the calculation a bit, since you would have greater investment in the project after the merge.

I think the best approach right with your wish to have i merged in the current state is:

Go through the last comments + Fix the linting errors (feel free to skip the G601 ones, since it will be fixed when we update to Go 1.22)
Mark the project as Beta in the Readme and in the description of the --platform flag/option.

lindell · 2024-07-01T20:27:00Z

cmd/platform.go

@@ -283,6 +286,37 @@ func createGiteaClient(flag *flag.FlagSet, verifyFlags bool) (multigitter.Versio
 	return vc, nil
 }

+// TODO: Add more code to the client


Please remove

lindell · 2024-07-01T20:32:23Z

docs/README.template.md

@@ -132,3 +132,27 @@ All configuration in multi-gitter can be done through command line flags, config
 {{end}}{{end}}

 Do you have a nice script that might be useful to others? Please create a PR that adds it to the [examples folder](/examples).
+


I think the documentation look good. But it is a bit too much to have in the main readme.

We could either:

Create docs/bitbucket-cloud.md and move most things there, while keeping a link to it here.

Use <details> to hide most info.

lindell · 2024-07-01T20:32:39Z

examples/go/upgrade-go-version.sh

@@ -2,5 +2,5 @@

 # Title: Upgrade Go version in go modules

-go mod edit -go 1.18
+go mod edit -go 1.19


ping on this comment

internal/scm/bitbucketcloud/bitbucket_cloud.go

lindell · 2024-07-01T20:40:11Z

internal/scm/bitbucketcloud/bitbucket_cloud.go

+	return scm.PullRequestStatusSuccess, nil
+}
+
+func (bbc *BitbucketCloud) GetOpenPullRequest(ctx context.Context, repo scm.Repository, branchName string) (scm.PullRequest, error) {


But if there is no intention to have it have it be used outside, please just make it private in that case.

lindell · 2024-07-01T20:47:59Z

internal/scm/bitbucketcloud/bitbucket_cloud.go

+
+func (bbc *BitbucketCloud) MergePullRequest(ctx context.Context, pr scm.PullRequest) error {
+	bbcPR := pr.(pullRequest)
+	repoSlug := strings.Split(bbcPR.guiURL, "/")[4]


Is this not the same as bbcPR .repoName? If not please create a function to extract the slug from the PR and describe what this splitting is doing (what is the 4 previous parts of the url?).

I mentioned it in a resolved comment but no, when I tried to use bbcPR.repoName it did not work and this slug is different. I used a clear variable name to clear up confusion

The previous 4 parts are the complete URL but for the repo slug we only care about the repo name which we are extracting from the URL in this case

lindell · 2024-07-01T20:48:06Z

internal/scm/bitbucketcloud/bitbucket_cloud.go

+
+func (bbc *BitbucketCloud) ClosePullRequest(ctx context.Context, pr scm.PullRequest) error {
+	bbcPR := pr.(pullRequest)
+	repoSlug := strings.Split(bbcPR.guiURL, "/")[4]


Is this not the same as bbcPR .repoName? If not please create a function to extract the slug from the PR and describe what this splitting is doing (what is the 4 previous parts of the url?).

lindell · 2024-07-01T20:58:14Z

internal/scm/bitbucketcloud/bitbucket_cloud.go

+	return err
+}
+
+func (bbc *BitbucketCloud) GetRepositories(ctx context.Context) ([]scm.Repository, error) {


I'm hesitant to add this as this would heavily limit the expected behaviour. But think it could be ok if

This is clearly described in the Readme (or wherever the info is moved) under the restrictions sections.

Mark it as beta (by adding (beta) to the bitbucket cloud section.

lindell · 2024-07-01T20:59:49Z

internal/scm/bitbucketcloud/custom_structs.go

+}
+
+// repository contains information about a bitbucket repository
+type repository struct {


Fine for now. Just would make it easier to make those changes later

waded · 2024-07-11T17:34:25Z

I tested this, and was successful in creating some PRs across multiple repositories. Love it!

Two areas for improvement:

The error when --org isn't set, panic: runtime error: index out of range [0] with length 0, isn't intuitive. If it's required it'd be nice if the error explained this. I'm new to multi-gitter, and so unsure if this is an issue w/ all platforms, or is specific to bitbucket_cloud.

We have 1000s of repositories in Bitbucket Cloud. Although I provided the names of fewer than 10 to --repo I did receive a could not fetch repositories: 429 Too Many Requests after a few dry runs (which each took far longer than I expected.) After letting things cool down, got past the 429, but still it was pretty slow. The 10 repos in question are very small. Is there perhaps something non-optimal about the number of calls it makes to Bitbucket Cloud?

gcase555 · 2024-07-15T22:41:04Z

As per the pervious comment, I did a pass and tried to be non-nitty.

Just to clarify, you are employees of Atlassian? It does change the calculation a bit, since you would have greater investment in the project after the merge.

Correct, we are developers from Atlassian and we use golang heavily on some platform tools we released here https://github.com/asecurityteam/ we still plan on submitting some smaller follow up PRs but we are trying to get this merged with the working functionality in place since this PR is getting to large and been open for longer then we prefer.

I think the best approach right with your wish to have i merged in the current state is:

1. Go through the last comments + Fix the linting errors (feel free to skip the G601 ones, since it will be fixed when we update to Go 1.22)

2. Mark the project as Beta in the Readme and in the description of the `--platform` flag/option.

I addressed all the nits, linter fixes and added a note on Beta support in the README in our fork here asecurityteam#7 I plan on requesting a re-reviewing once I get that merged.

gcase555 · 2024-07-15T22:44:27Z

I tested this, and was successful in creating some PRs across multiple repositories. Love it!

I am really glad to hear that!

The error when --org isn't set, panic: runtime error: index out of range [0] with length 0, isn't intuitive. If it's required it'd be nice if the error explained this. I'm new to multi-gitter, and so unsure if this is an issue w/ all platforms, or is specific to bitbucket_cloud.

Noted, this is something we will probably improve in a small follow up PR

Two areas for improvement:
We have 1000s of repositories in Bitbucket Cloud. Although I provided the names of fewer than 10 to --repo I did receive a could not fetch repositories: 429 Too Many Requests after a few dry runs (which each took far longer than I expected.) After letting things cool down, got past the 429, but still it was pretty slow. The 10 repos in question are very small. Is there perhaps something non-optimal about the number of calls it makes to Bitbucket Cloud?

We did make a note in the README that workspaces with lots of repositories will perform slower, it will still work but there is usually a delay when initially doing look ups for repos and PRs, you should still see commands execute within 2 minutes. We noted this as part of the Beta support that it does work but the performance will likely get better once we add more options for project support. With smaller workspaces, it runs very fast so this is something specific to large workspaces.

More feedback

Fix conflicts

gcase555 · 2024-07-16T17:57:02Z

Alright @lindell ready for a re-review, we addressed all linting issues besides the ones you mentioned we could ignore, did the requested cleanup of TODOs, added the function for extracting the repo slug, made notes on beta support(mentioned the issue with it being slightly slower on larger workspaces until project support gets added) and updated the docs to use details to make them more minimal.

gcase555 · 2024-07-31T20:09:41Z

@lindell is it possible we could get help merging this?

lindell · 2024-08-05T19:52:38Z

I'm about to merge this PR. But I can push the go mod changes necessary (fixes for merge conflicts).

It seems that "Maintainers are allowed to edit this pull request." is enabled, but I get

ERROR: Permission to asecurityteam/multi-gitter.git denied to lindell.

When I try to push the changes.

kpocius · 2024-08-05T19:57:52Z

I'm about to merge this PR. But I can push the go mod changes necessary (fixes for merge conflicts).

It seems that "Maintainers are allowed to edit this pull request." is enabled, but I get
ERROR: Permission to asecurityteam/multi-gitter.git denied to lindell.
When I try to push the changes.

This is a known limitation of GitHub. That setting doesn't work if the repository belongs to an organization.

https://github.com/orgs/community/discussions/5634

gcase555 · 2024-08-05T20:36:49Z

Ill address the merge conflicts now

Fix conflicts 2

gcase555 · 2024-08-05T21:10:24Z

@lindell all merge conflicts have been addressed

github-actions · 2024-08-06T05:56:44Z

Included in release v0.53.0 🎉

lindell · 2024-08-06T05:56:48Z

Thanks for the contribution. I am looking forward to the fixes coming soon 🙂

gcase555 and others added 17 commits March 22, 2024 15:22

init empty files

3d51a03

adding init methods

ed6e44b

tidying up

ecbbfc2

added go-bitbuckety

08e2f99

adding init settings function

f9d80fb

adding init config

0cf3f8b

updating settings for client

ce8d671

Added Bitbucket cloud platform init, authentication as well implement…

c6d00fe

…ation for create and get pull requests

trying to add merge PR

0037e33

removing unused field

7e4e678

fixing merge code, got working merges for most of the PRs

4da8faf

adding close PR code

1e99218

get rid of comment

bca67e4

remove unused string

c20dfb5

Merge pull request #1 from gcase555/bitbucket-init

3247873

WIP: Adding bitbucket support

add fork and update PR

0c42004

Merge pull request #2 from gcase555/update-PR-fork-support

3340226

add BBC fork and update PR support

gcase555 commented May 13, 2024

View reviewed changes

internal/scm/bitbucketcloud/custom_structs.go Show resolved Hide resolved

lindell reviewed May 14, 2024

View reviewed changes

lindell reviewed May 15, 2024

View reviewed changes

lindell reviewed May 20, 2024

View reviewed changes

updates from upstream

529ce19

gcase555 force-pushed the master branch from 73a4afe to 529ce19 Compare June 12, 2024 19:39

byott and others added 5 commits June 13, 2024 12:26

update docs and improve fork support during PR creation

1013cba

Merge pull request #3 from gcase555/bbc-docs-and-create-pr-fix

cf69583

Update Docs and improve PR creation during forks

fixing issue where current user is a default reviewer

0a80442

added error handling

97d129b

removing println

471bfb9

gcase555 requested a review from lindell June 25, 2024 02:06

lindell reviewed Jul 1, 2024

View reviewed changes

gcase555 added 6 commits July 15, 2024 16:11

fixing script and updating README

ca93a8f

fixing comment

5c7bdef

adding beta comments

e122da6

fix all linters

e799052

adding comments

ce58a71

adding function for extracting slug name

7a8fb81

gcase555 and others added 5 commits July 15, 2024 17:50

updates from master

16e4493

updates from master

f84981a

Merge pull request #7 from asecurityteam/more-feedback

9928e39

More feedback

Merge branch 'master' of github.com:asecurityteam/multi-gitter

6fbd56c

Merge pull request #8 from asecurityteam/fix-conflicts

e1bdb9b

Fix conflicts

gcase555 requested a review from lindell July 16, 2024 17:55

gcase555 and others added 2 commits August 5, 2024 15:57

fixing conflicts and running go mod tidy

a3125c3

Merge pull request #10 from asecurityteam/fix-conflicts-2

de1ec4a

Fix conflicts 2

lindell merged commit cbe65fa into lindell:master Aug 6, 2024
9 checks passed

github-actions bot mentioned this pull request Aug 6, 2024

chore(master): release 0.53.0 #496

Merged

		@@ -132,3 +132,27 @@ All configuration in multi-gitter can be done through command line flags, config
		{{end}}{{end}}

		Do you have a nice script that might be useful to others? Please create a PR that adds it to the [examples folder](/examples).

Adding bitbucket cloud support #479

Adding bitbucket cloud support #479

Conversation

gcase555 commented May 13, 2024

What does this change

What issue does it fix

Notes for the reviewer

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lindell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gcase555 commented May 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gcase555 commented Jun 25, 2024

lindell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waded commented Jul 11, 2024 • edited Loading

gcase555 commented Jul 15, 2024

gcase555 commented Jul 15, 2024

gcase555 commented Jul 16, 2024 • edited Loading

gcase555 commented Jul 31, 2024

lindell commented Aug 5, 2024

kpocius commented Aug 5, 2024

gcase555 commented Aug 5, 2024

gcase555 commented Aug 5, 2024

github-actions bot commented Aug 6, 2024

lindell commented Aug 6, 2024

waded commented Jul 11, 2024 •

edited

Loading

gcase555 commented Jul 16, 2024 •

edited

Loading