go-bitbucket
go-bitbucket copied to clipboard
ktrysmt
Ddansby/issue 154 add delete branch and tag methods
This PR closes #154. It specifically adds the DeleteBranch and DeleteTag methods.
Note this PR should not be merged until https://github.com/ktrysmt/go-bitbucket/pull/155 is merged since it uses the code from that PR for its tests.
It also works towards removing branch and tag specific options and moving to a single refs options (see https://github.com/ktrysmt/go-bitbucket/issues/153). Furthermore This test also improves the ListRefs tests from https://github.com/ktrysmt/go-bitbucket/pull/155 by adding a part that tests the creation of a Tag and then also tests the new DeleteBranch and DeleteTag methods as they are used in the tear down of the ListRefs tests (they are used to delete the test tag/branch).
I believe this is good to be approved/merged, but I am interested to hear opinions about how I currently handle errors/responses from DELETE http methods. Currently, the Bitbucket API returns an error json/map[string]interface{} type if there is an error and returns and 204 response code if the DELETE is successful.
Currently, in this PR I just processed and returned interface{} similarly to the Delete method for the repository type itself. Do you think we should leave it to the user to handle error responses? Or should I try to handle it more gracefully? Should I worry about providing more intuitive success responses; or just leave it as a typical delete method in that only return a response if there is an issue? Currently, for testing, I search that the response doesn't include the error type which is indicative that there was an issue deleting a branch/tag for any of the reasons specified in the documentation (for example https://developer.atlassian.com/bitbucket/api/2/reference/resource/repositories/%7Bworkspace%7D/%7Brepo_slug%7D/refs/tags/%7Bname%7D#delete)
Hi, I merged https://github.com/ktrysmt/go-bitbucket/pull/155 and please rebase your branch. @DataDavD
hey @ktrysmt FYI, not sure if you saw my last message, but the branch/PR should be good to merge now. let me know if you need anything else.
Hi, @DataDavD . It would be helpful for me if you could rebase and summarize the commits.
hey @ktrysmt I'm a bit of a rebase noob tbh. I had planned to start rebasing from my initial commit for this branch (i.e. 2f8578e1728018e7204ea07b7b5cd51359cdcc9b) but after reading some documentation I am confused if that would be the correct way to rebase given lots of documentation recommends avoiding rebasing commits that have already been pushed to protected branches since it would cause duplicate commits. I know this isn't necessarily isn't a bad thing; I just want to make sure that is correct/fine with you or if I am doing it incorrectly (i.e. from wrong commit or something).
Thanks in advance for your help and patience.
Best, dd
@DataDavD Thanks for your operation. Sorry maybe rebase is not necessary. I think what this probably need squash of commits.
No worries at all. I think you are right. However, going forward, I will start squashing any commits/merge commits I have before pushing to the remote so that the history is a little cleaner for my PRs.
Let me know if you need any more help from me on this.
thanks again!
Hey @ktrysmt, I was wondering if you started thinking about how we can merge this PR without breaking existing code. Since this combines Tag/Branch options into the single RefOptions this will cause breaking changes. However, I think its beneficial as it simplifies the code and closer aligns with git data model.
What is the process in go-bitbucket for implementing breaking changes? Do you think this breaking change is big enough to do a major semantic version incrementation? Or should I continue to try and find more places where we could do similar code changes as I have done here?
@DataDavD Certainly this PR contains destructive changes. However, the major version number for this project is v0. Therefore, it is permissible to add destructive changes to the mainline.
But it's not so good to release it without checking it in detail. So, I'll try to test it.
By the way, this branch has conflicts, please resolve it and push the clean codes. @DataDavD
Sounds good @ktrysmt , completely agree with you. I can assist with any test additions as well.
Hey @ktrysmt im out of town but will be back Monday and be able to resolve the conflicts then. I believe it's related to someone getting a PR merged for their version of BranchDelete but should be able to easily combine theirs with these ref additions/changes.
I'll let y'a know onces their resolved and the PR is good again.
Thanks again!!!
hey @ktrysmt sorry for the delayed action. I had to travel again and have been very busy with work and life (holidays are happening around here) so I haven't finished resolving the conflicts yet. I was about done but realized I messed up one of my applied changes during my rebase merge so I need to redo it again. I should be done tomorrow.
However, I'm curious if @chmouel would be affected if we reverted his DeleteBranch functionality to my original implementation provided in this PR (The main conflict is the fact that the DeleteBranch from @chmouel was approved/merged after this PR was approved (but not merged), so need to decide the original implementation; I'm inclined to opine towards my implementation). It should be fine, but I wanna make sure it won't impact @chmouel in a negative manner (would also appreciate your thoughts here @ktrysmt).
@DataDavD thanks for letting me know! yeah my code will probably be affected but i'll update it! cheers 👍🏻
No problem to override, it's a good change that follows the API spec. @DataDavD And thank you. @chmouel
It would be nice for me if you could do some test just to be sure after the conflict's resolution.
Sounds good @ktrysmt. Sorry for the delay, been super busy with life and work and haven't been able to do any personal work. I should have this done before next weekend, most likely much earlier. I'll let you and @chmouel know when it's ready so that @chmouel can take a look and have a buffer for him to adjust any code impacted by this change.
@ktrysmt FYI I have resolved the conflicts and reran my test to success.
@chmouel notice the existing changes. I imagine the only changes on your end would be using RepositoryBranchOptions instead of RepositoryBranchDeleteOptions
@DataDavD that's correct! my project is go.modded so when i'll start updating with go get -u the change I'll see the conflict quickly at compile time! thanks
hey @ktrysmt sorry for spam, but I was wondering if you saw this, wanna try and merge before more conflicts arise. Happy to discuss merges/update options async here too