Respect GitLab's delete source branch checkbox

[UnsolvedCypher]

Currently, acceptGitLabMR passes false to GitLab's API for should_remove_source_branch unless otherwise specified. This change makes it specify null instead, which means that unless it is specified, it will respect the checkbox in the GitLab UI for removing the source branch after merge.

[basil]

Incremental build 1.5.28-rc1334.943db_b_50c532 is available for testing. The incremental build is available from: https://repo.jenkins-ci.org/incrementals/org/jenkins-ci/plugins/gitlab-plugin/1.5.28-rc1334.943db_b_50c532/

@UnsolvedCypher Can you please test the incremental build and confirm the issue is resolved? For instructions on how to install a custom build, see: https://www.jenkins.io/doc/book/managing/plugins/#advanced-installation

[UnsolvedCypher]

Hello @basil, thank you for the instructions. I have set this up locally and confirmed that this change does in fact work and causes GitLab's "Should remove source branch" checkbox to be respected.

In the process, I found that in the plugin even without my change completely ignores the removeSourceBranch parameter to acceptGitLabMR(). Before my change, it would always pass "false" to GitLab regardless of the parameter, and now with my change, it always passes "null" (which means that it defers to GitLab's checkbox).

I can open a separate issue for that, but as this PR does at least improve the situation, I think it is ready to be merged.

[basil]

Can you please look into fixing acceptGitLabMR so that the removeSourceBranch parameter is properly propagated to the GitLab API? I fear our understanding of the problem is incomplete, and I want to get a better understanding of what is going on before releasing this.

[UnsolvedCypher]

I'd be happy to look into it but I will need some help understanding the architecture. I don't really understand how the parameters from acceptGitLabMR get translated as there isn't any function in the code called acceptGitLabMR. It looks like there is some magical stuff happening through annotations. Could you give me some pointers on where to look or what to search for?

[basil]

Sure, the relevant source file is AcceptGitLabMergeRequestStep which implements the Pipeline: Step API (see also this page). This is annotated with @DataBoundSetter:

    @DataBoundSetter
    public void setRemoveSourceBranch(boolean removeSourceBranch) {
        this.removeSourceBranch = removeSourceBranch;
    }

It seems like one problem is that this uses the mechanism for optional parameters (@DataBoundSetter) as well as the mechanism for mandatory parameters (@DataBoundConstructor) described in the docs. I can't quite tell what's truly mandatory and what's optional from reading the code, but if all parameters are optional then we should remove the @DataBoundConstructor annotation and replace it with @Deprecated. If some parameters are indeed mandatory, then we should have a @DataBoundConstructor with just those mandatory parameters. There should be no overlap between the parameters covered by @DataBoundConstructor and the parameters covered by @DataBoundSetter. Hope this helps get you started.

[UnsolvedCypher]

Thank you, that is very helpful. I have determined that the request getting sent to GitLab is correct, but for some reason is still not working properly. There have been a number of bugs filed and fixed in the past few years about GitLab's should_remove_source_branch API parameter. I suspect I may be hitting one, and the version of Dockerized GitLab that this repo uses is quite old.

I've opened #1232, so if you are alright with it, I will open a PR to upgrade the docker-compose to a more modern version and see if that fixes this problem.

[basil]

I've opened #1232, so if you are alright with it, I will open a PR to upgrade the docker-compose to a more modern version and see if that fixes this problem.

Sure, go ahead!

[UnsolvedCypher]

Hi @basil, I believe this is ready to be merged. I have made some further changes (part of the plugin was still using primitive boolean rather than Boolean), and the updated GitLab in the Dockers seemed to do the trick.

I have ensured (by building an hpi locally and installing it on a Dockerized Jenkins) that:

1. Passing false to should_remove_source_branch always preserves the branch
2. Passing true to should_remove_source_branch always deletes the branch
3. Not passing anything to should_remove_source_branch defers to GitLab's checkbox

Please let me know if there are any other changes you would like to this PR!

[basil]

Very nice! Thanks again for the PR.

[UnsolvedCypher]

Wonderful, thank you for the quick merge! Would it be possible to get a release sometime soon? This fix would be very helpful to have at work.

[basil]

Released in 1.5.28.