Don't check LastCommitNotYetBuild on Approve MR action

[jakub-bochenski]

Related to #659
I think it makes sense to re-trigger builds on approve even if they were already built.

We have builds that will only do certain actions for approved MRs.
Without this the build would run before the MR was approved and would never execute the approved-only actions.

Nov 14, 2017 4:15:43 PM INFO com.dabsquared.gitlabjenkins.trigger.handler.merge.MergeRequestHookTriggerHandlerImpl isLastCommitNotYetBuild

Last commit in Merge Request has already been built in build #119

[jakub-bochenski]

@omehegan can you let me know if this change is going in the right direction?
I don't like how complex the condition is now, but I don't see any better way to achieve that

[jakub-bochenski]

Another approach would be to add a separate option "Approved Merge Request Events" like the "Opened Merge Request Events" and "Accepted Merge Request Events".

However this would require a change to the flow described above to split off the approved-only part into a separate job.

[omehegan]

@jakub-bochenski This seems like a good improvement. Out of curiosity, what are you checking in your logic about whether the MR is approved? The gitlabActionType variable? Have you already tested this change?

@mreichel or @Argelbargel do you have any comments on this? The actual diff is small, but @jakub-bochenski is right that the conditional statement here is pretty complex. I don't know if it can be simplified or if it would be worth trying.

[Argelbargel]

This change would be a good addition to make it possible to e.g. create release-builds after approval or something similar. i think there should be an option to disable building on/after approval for those use-cases where you just want to check whether everything is okay before approving a merge-request.

As for the conditions compexity: the additional check does not make it that worse - for a start i would refactor the checks into their own method (shouldBuild() or something) and leave it at that...

[jakub-bochenski]

Out of curiosity, what are you checking in your logic about whether the MR is approved? The gitlabActionType variable?

@omehegan actually I haven't thought of that. What we do now is we only grab the MR id from environment variable and then use Gitlab API for everything else. This makes testing outside of Jenkins easier. Your approach has the advantage of being faster and more atomic.

PS. The value of gitlabActionType is still only MERGE so that wouldn't work

[jakub-bochenski]

Have you already tested this change?

I tested it today and it's broken :( I'm seeing the hook is received in the log, but then nothing not even the "Last commit in Merge Request has already been built in build".

I will investigate this tomorrow

Nov 15, 2017 5:37:25 PM INFO com.dabsquared.gitlabjenkins.webhook.GitLabWebHook getDynamic
WebHook called with url: /project/gitla-labels-test

Nov 15, 2017 5:37:25 PM FINE com.dabsquared.gitlabjenkins.webhook.build.MergeRequestBuildAction 
MergeRequest: {

[jakub-bochenski]

@omehegan this works after all: turns out we had a secret mismatch between the job config and gitlab trigger

Nov 20, 2017 1:00:33 PM INFO com.dabsquared.gitlabjenkins.trigger.handler.AbstractWebHookTriggerHandler handle

gitla-labels-test triggered for merge request.

[jakub-bochenski]

I think I will rewrite this to have a separate trigger option like it was done in #508

[jakub-bochenski]

Tested manually, with this settings MRs will trigger only on MR approve (and not e.g. on push to source branch)

Will add automated test coverage tomorrow

[jakub-bochenski]

@omehegan I'm done, can you please review?

Can you please also check #671 - it makes doing any changes very cumbersome

[omehegan]

@jakub-bochenski can you resolve merge conflicts? Then ping @Argelbargel or @mreichel for review, I unfortunately won't have much time for the plugin for the next couple of weeks.

[jakub-bochenski]

@Argelbargel or @mreichel please review

[Argelbargel]

@jakub-bochenski: do your changes cause the test-failures or is this "just" a flickering test?

[jakub-bochenski]

@Argelbargel looks random
I didn't touch this part and 1aadd46 was green

Actually master is red since f8c9cbd

[mreichel]

It is red since f8c9cbd because of failing dependency check (@Argelbargel shouldn't we get rid of the dependency check? It does not work properly and confuses everyone). The tests fail since 4d50561, I will look into it.

[Argelbargel]

@mreichel: do you mean the check running when pushing to a pr? yes please!

[jakub-bochenski]

@Argelbargel the build is now green, can I get a review please?

[jakub-bochenski]

Last build failed because of some problems with Jenkins Azure host java.nio.channels.ClosedChannelException

[Argelbargel]

I think the changes are okay

[jakub-bochenski]

@Argelbargel or @mreichel can you merge this?