search

smarkets / marge-bot

A merge-bot for GitLab
BSD 3-Clause "New" or "Revised" License
701 stars 136 forks source link

Only delete source branch if it was forced. #193

Closed Viatorus closed 4 years ago

Viatorus commented 5 years ago

If the option Delete source branch when merge request is accepted. is not checked, the branch will not be removed.

Viatorus commented 5 years ago

WTF: R: 11, 0: Too many public methods (31/30) (too-many-public-methods)

Viatorus commented 5 years ago

@JaimeLennox Could we merge this?

JaimeLennox commented 4 years ago

Sorry it took so long to get to this!

I've just had a great time reading through why there are two options called force_remove_source_branch and should_remove_source_branch, and I'm more confused then when I started.

The best information I could find is from https://gitlab.com/gitlab-org/gitlab/issues/18283#note_214998578, which seems to indicate that force_remove_source_branch is the option on the merge request and should_remove_source_branch is for merge time.

And if this wasn't enough the Gitlab API docs also have a plain remove_source_branch option for when updating the MR: https://docs.gitlab.com/ee/api/merge_requests.html#update-mr

In any case, we use should_remove_source_branch for accepting the MR, and this seems to be the right one for that particular endpoint. Setting this to the same option as force_remove_source_branch seems fine, as we're just respecting the user's wishes, although I also think removing the parameter would have the same effect since then we're not overriding anything at merge time. This also seems a bit cleaner than messing around with these options in the first place, given they're quite confusing!

What do you think?

Viatorus commented 4 years ago

I just remember, if someone says the branch should not be deleted, it was deleted anyways.

The GUI checkbox option remove source branch inside a MR is ignored if you merge via remote API.

JaimeLennox commented 4 years ago

I see! Well that just adds to the fun I guess. I'm happy to merge this in then.