lindell / multi-gitter

Update multiple repositories in with one command
Apache License 2.0
866 stars 64 forks source link

Adding bitbucket cloud support #479

Closed gcase555 closed 1 month ago

gcase555 commented 4 months ago

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.

gcase555 commented 4 months ago

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.

gcase555 commented 3 months ago

@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.

waded commented 2 months ago

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 commented 2 months ago

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 https://github.com/asecurityteam/multi-gitter/pull/7 I plan on requesting a re-reviewing once I get that merged.

gcase555 commented 2 months ago

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.

gcase555 commented 2 months ago

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 commented 2 months ago

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

lindell commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

Ill address the merge conflicts now

gcase555 commented 1 month ago

@lindell all merge conflicts have been addressed

github-actions[bot] commented 1 month ago

Included in release v0.53.0 🎉

lindell commented 1 month ago

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