Implement approval for merge requests

[padyx]

This PR adds a new post-build action to the plugin: The ability to approve / unapprove a merge request depending on the build result (resolves issue #95). It offers a configuration option whether unstable builds should be approved anyway (default: no).

Note: The GitLab approval feature is not available in the self-hosted Community Edition (only Enterprise Edition Starter, or above). The approval feature is available for all public repositories on GitLab.com or in the Bronze tier, or above.

The basic implementation works, but there are several points in need of discussion in this PR before it is ready for merging:

• [x] Approvals are only available in the API v4. How can we handle it - or should we at all handle the case that a v3 API is used?
• [X] Is the "EE only" warning in the post-build action title and help sufficient?
• [x] The API description doesn't mention HTTP result codes and the actual result body contains undescribed elements. The post-build action should only try to un-/approve, if the API user can do so (is an eligible approver) and hasn't done so yet.

My experimentation yielded these results so far, the last two

• Approvals are unsupported: 404
• Not an eligible approver: 401
• Approving if the request is already approved: 401
• Unapproving if the request is already approved: 404 (!)
• Un-/Approving if everything works: 201

The result body contains two very useful attributes, but these aren't mentioned in the API docs - so I'm unsure about relying on them since I don't know if they are available since 8.9: user_has_approved and user_can_approve.

I'd appreciate your input.

Acceptance criteria so far:

• [ ] your changes work with the oldest and latest supported GitLab version
• [x] new features are provided with tests
• [ ] refactored code is provided with regression tests
• [ ] the code formatting follows the plugin standard
• [x] imports are organised
• [x] you updated the help docs
• [ ] you updated the README
• [ ] you have used findbugs to see if you haven't introduced any new warnings

Fixes #95

[omehegan]

@padyx thanks for submitting this!

I'm happy with the help text that makes clear that this will only be supported in GitLab EE. Regarding the API v3 vs v4 question, I think you should figure out what version of GitLab introduced the API we are using, and then add that as a requirement in the help text. E.g. 'EE v9.0 or newer only.'

@dblessing can you or someone else from GitLab comment about the HTTP result codes that should be expected/are possible here, and also the user_has_approved and user_can_approve attributes?

[omehegan]

@dblessing ping!

[dblessing]

API v4 was introduced in 9.0. v3 was removed in 11.0. That doesn't mean all features of v4 existed in 9.0, though.

Those HTTP codes look to be comprehensive for the endpoints you're using.

[padyx]

Thanks for the input @dblessing

I've used git bisect to figure out when the properties user_can_approve and user_has_approved were introduced. Both were present in GitLab (EE) commit 85409f275e75506eb6dea1a2985b6f644a99812e, which is contained in v8.17.0-ee.

Combined that v4 API is available from GitLab EE 9.0 and newer, and that the API v3 does not support approvals at all , I should be able to use the two properties. I'll adapt the PR to use the two properties for better handling in advance, instead of only trying to interpret the response codes after the attempt to approve.

@omehegan Do you have any preference how to handle the unsupported methods on the V3GitLabApiProxy? If I remove them, RESTEasy throws a RuntimeException in the tests since the methods in the GitLabApiProxy have no annotations.

RESTEASY004600: You must use at least one, but no more than one http method annotation on: public abstract void com.dabsquared.gitlabjenkins.gitlab.api.impl.GitLabApiProxy.approveMergeRequest(java.lang.Integer,java.lang.Integer)

[omehegan]

@padyx TBH I'm not informed enough to have a preference :) If you want to move ahead and just use your own judgment, that's fine with me.

[padyx]

I've attempted to implement the other version using the canApprove/hasApproved flags . In the current version it is not well tested and has two issues I'm not happy with:

1. It requires an additional API call (the canApprove/hasApprove flags are not on the main MR return result). This introduces an additional potential point of failure in the interaction between plugin and GitLab.
2. I haven't (yet) managed to make the unit tests work by mocking the additional API call to retrieve that info - so I don't feel confident in the second version.

For now, I've cleaned up the remaining FIXMEs in the PR and added the info that approvals are available in EE 9.0 and up (since that is when API v4 was introduced). I'd suggest that we merge the current implementation without the flags (and add the flag checking if necessary at a later point in time). It would be great if someone could test the build too!

[omehegan]

I've uploaded a snapshot with this patch to http://repo.jenkins-ci.org/snapshots/org/jenkins-ci/plugins/gitlab-plugin/1.5.12-SNAPSHOT/gitlab-plugin-1.5.12-20181227.052257-3.hpi. I will work on testing it myself. If anyone else is willing to try it, please do.

[erbrecht]

I've installed the snapshot version you linked, and all is working with jenkins 2.190.1 and GitLab Enterprise Edition 12.6.4-ee. It approved my merge request as the integration user I configured.

I'll gladly do some more testing if you let me know what specifically to look for. I'm really hoping to get this into our production system.

[erbrecht]

@padyx @omehegan I know this pull request is over a year old and I was wondering if it is still being considered.

[padyx]

@erbrecht I'm still hoping it'll get merged. Initially I had wanted to rewrite it slightly to consider the new flags, but lacked the time to rewrite it with the proper tests.

@omehegan What's your opinion on merging this?

[Alceatraz]

Any update about this? In plugin 1.5.35 still no "Approve GitLab merge request on success"

[padyx]

It's been a while and I don't use Jenkins at my current position so I'm no longer pushing this actively. But I'd still be willing to get this back into a mergeable shape, if a maintainer can tell me what is missing to get that done.

@omehegan Is anything missing apart from obvious conflicts that I'd need to fix?

Otherwise, I'd propose to close this.