Adds a Modern Mode handling for merge requests. #285

pbendersky · 2016-05-02T20:00:27Z

In Modern Mode, the plugin will fetch Merge Requests from origin instead of using parameterized builds.

This supersedes #252 and works with the refactored 1.2 codebase.

…lugin will fetch Merge Requests from origin instead of using parameterized builds.

pbendersky · 2016-05-02T20:02:59Z

Here's the original comment I wrote on #252:

This PR enhances gitlab-plugin by adding two operation modes: Modern and Legacy.
Legacy is what we currently have, that works with older versions of GitLab.

Modern works with GitLab 8.1 or better, and provides an easier configuration of projects in Jenkins (a single Git repository is enough) and much better handling of Merge Requests (MR are fetched from origin and matched by SHA1 instead of comparing branch names).

This PR fixes the two issues I reported: #239 and #155.

omehegan · 2016-05-02T21:54:09Z

@pbendersky thanks a lot for doing this work!

omehegan · 2016-05-02T22:03:26Z

src/main/java/com/dabsquared/gitlabjenkins/util/CommitStatusUpdater.java

@@ -5,6 +5,7 @@
 import com.dabsquared.gitlabjenkins.gitlab.api.GitLabApi;
 import com.dabsquared.gitlabjenkins.gitlab.api.model.BuildState;
 import hudson.EnvVars;
+import hudson.model.AbstractBuild;


Are you using AbstractBuild here? Note that, in order to support Pipeline jobs, AbstractJob and AbstractBuild can't be used. See https://github.com/jenkinsci/pipeline-plugin/blob/master/DEVGUIDE.md

I'm not. I'll add a commit removing it.

pbendersky · 2016-05-04T12:56:25Z

@coder-hugo @omehegan I'm keeping this PR updated with master, so it's easier to merge when you want to.
Please let me know if you want to do additional changes, or discuss any decisions I made.

omehegan · 2016-05-05T00:26:39Z

@pbendersky thanks! I'm going to try to test it in my environment tomorrow, where we use GitLab CE 7.14. That's probably the most I can do with it. @coder-hugo said that he will look through it as soon as he can.

# Conflicts: # src/main/java/com/dabsquared/gitlabjenkins/cause/CauseData.java # src/main/java/com/dabsquared/gitlabjenkins/trigger/handler/merge/MergeRequestHookTriggerHandlerLegacyImpl.java

…ild was initiated manually instead of through a GitLab trigger.

omehegan · 2016-05-13T22:44:02Z

@pbendersky I'm working on testing this. The only merge conflict is with changes to the README, so it should be easy to resolve that.

pbendersky · 2016-05-13T23:57:03Z

Nice! Let me know if you need anything, or you have comments on the code.

omehegan · 2016-05-14T00:07:44Z

@pbendersky there seems to be a bug in the existing HEAD of master, I'm getting an NPE when I trigger a build with that code. Unrelated to your change. I'll see if I can track it down.

tommyminds · 2016-05-17T13:57:03Z

Is there any update on the status of this PR? Or on the next release (1.2.3) in general? There are some fixes in master that would be good to have released.

omehegan · 2016-05-17T16:19:11Z

I determined yesterday that the current master has a bug that breaks compatibility with GitLab 7.14 and possibly everything before GitLab 8.5. I have a patch for it that just needs cleanup. I will merge that this week and then I can test this change.

omehegan · 2016-05-19T20:18:51Z

OK, I have successfully tested this with GitLab 7.14, using the legacy mode, and it correctly triggers a build based on push or MR, and checks out the right branch. So that much looks good to me.

omehegan · 2016-05-19T20:20:50Z

src/main/java/com/dabsquared/gitlabjenkins/GitLabPluginMode.java

+public enum GitLabPluginMode {
+    LEGACY,
+    MODERN;
+/*    


Do we need to keep all this commented out stuff?

omehegan · 2016-05-19T20:36:26Z

I posted some minor comments on the code. Also wanted to note that I tested with a Pipeline job and it works; this should imply that it will work with Freestyle jobs as well.

@coder-hugo pending comments from you before we merge this. I've reviewed as much as I think I can.

- Addressed comments in PR

pbendersky · 2016-05-19T20:53:47Z

@omehegan Just fixed these minor issues, and merged from upstream, to make things easier.

omehegan · 2016-05-19T21:07:40Z

README.md

@@ -21,7 +21,11 @@
    - [Parameterized builds](#parameterized-builds)
 - [Branch filtering](#branch-filtering)
 - [Build Tags](#build-tags)
+<<<<<<< HEAD


Looks like you committed the merge conflicts of README...

Sorry, just added a new commit fixing it.

omehegan · 2016-06-01T20:48:45Z

@coder-hugo ping

coder-hugo · 2016-06-05T16:20:00Z

...in/java/com/dabsquared/gitlabjenkins/trigger/handler/push/PushHookTriggerHandlerFactory.java

        List<PushHookTriggerHandler> result = new ArrayList<PushHookTriggerHandler>();
        if (triggerOnPush) {
            result.add(new PushHookTriggerHandlerImpl());
        }
-        if (triggerOpenMergeRequestOnPush == TriggerOpenMergeRequest.both) {
+        if (gitLabPluginMode == GitLabPluginMode.LEGACY && triggerOpenMergeRequestOnPush == TriggerOpenMergeRequest.both) {


With this you disable the functionality to rebuild open MRs if the target branch has changed for the modern mode. Why you are doing this?

Because that case is handled by the Merge Request hook instead (if a MR is created and a commit added, the MR hook is called again).

Nope. I just tested this with GitLab 8.7:

I configured the job in Jenkins as you described in the README

I configured my GitLab repo to send MR events to the Jenkins

I created a MR for a test branch into the master -> a new build was triggerd

I pushed some changes to the master (target branch) -> no build was triggered as no event was sent by GitLab

You are right. The case I was referring was new commits on the source branch (the user's fork). That gets handled by the MR trigger. I'll take a look at this, it might be a limitation of the "Modern" approach (if that's the case, I'll document it to reflect it).

@coder-hugo just looked a bit at this... seems to be a limitation of the Modern mode. Since the user only has one repo configured, a new commit to master in the target branch can't be merged to the MR by Jenkins.
I suggest:

Updating the Readme to reflect this.

Ideally (I'll check how can it be done) change the drop down when selecting Modern mode, so the user can only select "Source branch" on when to re-build.
Thoughts?

…ing open MR when pushing to source branch. - Changed the settings so when switching the plugin Mode the options for "triggerOpenMergeRequest" are refreshed and show only the possible values.

pbendersky · 2016-06-06T19:35:57Z

@coder-hugo just added a two commits:

I clarified in the Readme that in Modern mode only pushes to the target repo will trigger a rebuild.
I added a target value to TriggerOpenMergeRequest.java
I changed GitLabPushTrigger.java so it reloads the options for TriggerOpenMergeRequest when you change the plugin mode and only shows the valid values for each mode.
Hope this makes it clearer to users.

I also merged the latest changes from master, so it passes the checks here.

coder-hugo · 2016-06-07T19:20:06Z

@pbendersky it looks like you have interchanged the meaning of source and target branch of a MR. The source branch is the one that contains the changes that shall be merged to the target branch. So the modern mode supports re-building open MRs by pushes to the source but not to the target branch. And as the target branch is part of the configured repository it should be possible to re-build open MRs also for pushes to the target branch.

pbendersky · 2016-06-07T19:42:32Z

Oh... I'll revert the source / target then.

So, a better approach (that would cover all the cases) would be to instead of fetching from merge-requests/x to fetch from origin and merge the contents of merge-requests/x. That way, commits added to either source or target would get applied. Would that work? (of course, respecting the user settings for this).

pbendersky · 2016-06-07T19:46:45Z

@coder-hugo I see one problem, however. Let's say there are 3 merge requests open, and a user commits to master in the upstream repository. How many builds should that trigger? If this works as you suggest, we would need to trigger four builds: one for the push to the upstream repo, and 3 for the updated merge requests. Is that correct?

omorillo · 2016-06-07T20:38:09Z

@pbendersky I don't see any problem there. You would like to know if the master is still stable but also if the source branches can be safely merged to the target branch.

pbendersky · 2016-06-08T13:27:00Z

@omorillo @coder-hugo I'm trying to wrap this up... Pending tasks (as discussed):

Merge origin with the merge request before build.
Add ability to trigger multiple builds on open merge requests when pushing to the target repo.

Now I need some help, and maybe you can give me a pointer... We currently create a RevisionParameterAction to tell Jenkins what revision to checkout. Is there any Action I can use to tell it to merge two repositories? I could not find it, and I'm not that familiar with Jenkins' API yet.

coder-hugo · 2016-06-15T18:42:39Z

@pbendersky you don't need to add a special Action to the build for merging two branches. This will be configured as additional behavior (Merge before build) of the git SCM config. (see here)

pbendersky · 2016-06-15T18:45:32Z

@coder-hugo right, but it loses some of the charm. As it is now, you simply set up origin on Jenkins and it will know how to fetch the merge requests from it. I'd like to keep that simplicity if possible. Maybe adding a second repository on build time? Other idea?

coder-hugo · 2016-06-15T19:09:16Z

@pbendersky you can try to extend the PreBuildMerge of the git-plugin and pass the required UserMergeOptions for GitLab within the constructor to the super constructor. This extended class should then appear as a new additional behavior for the git SCM. This should keep the job setup as simple as possible.

# Conflicts: # README.md # src/main/java/com/dabsquared/gitlabjenkins/GitLabPushTrigger.java # src/main/java/com/dabsquared/gitlabjenkins/trigger/handler/merge/MergeRequestHookTriggerHandlerFactory.java # src/main/java/com/dabsquared/gitlabjenkins/trigger/handler/merge/MergeRequestHookTriggerHandlerLegacyImpl.java # src/main/java/com/dabsquared/gitlabjenkins/trigger/handler/push/PushHookTriggerHandlerFactory.java # src/main/java/com/dabsquared/gitlabjenkins/util/CommitStatusUpdater.java # src/test/java/com/dabsquared/gitlabjenkins/publisher/GitLabCommitStatusPublisherTest.java # src/test/java/com/dabsquared/gitlabjenkins/trigger/handler/merge/MergeRequestHookTriggerHandlerImplTest.java

pbendersky · 2016-06-30T18:10:38Z

@coder-hugo @omehegan please take a look at #370. While I'm not giving up on this PR (I still think the setup with a single repository is easier to manage), the minor changes in configuration explained in #370 achieve the same result.

pabloa · 2016-08-03T21:00:14Z

Hello, any news with this Pull Request? I am interested because #98

pbendersky · 2016-08-03T21:01:38Z

I think it's a dead end at this point... With the configuration now in the README, I don't think this is needed. Did you try that?

omehegan · 2016-09-15T22:03:56Z

Should we make a call on this one? It sounds like we should close it. Agreed?

pbendersky · 2016-09-16T11:37:09Z

Agreed.

Merge in latest from jenkinsci

Merge pull request jenkinsci#285 from jenkinsci/master

pbendersky added 3 commits May 2, 2016 16:39

Adds a Modern Mode handling for merge requests. In Modern Mode, the p…

c862248

…lugin will fetch Merge Requests from origin instead of using parameterized builds.

Fixes for compatibility with latest master changes

238653b

Updated for compatibility with latest changes

ffdb6a5

pbendersky mentioned this pull request May 2, 2016

Plugin modes for better MR handling. #252

Closed

omehegan reviewed May 2, 2016
View reviewed changes

pbendersky added 3 commits May 2, 2016 19:49

Removed unused import of hudson.model.AbstractBuild

2418141

Updated README with Modern / Legacy explanation

91db58f

Merge branch 'master' into merge-requests-1.2

93bdb85

Merge remote-tracking branch 'upstream/master' into merge-requests-1.2

1ddb319

pbendersky added 3 commits May 9, 2016 15:25

Merge branch 'master' into merge-requests-1.2

e8fc119

# Conflicts: # src/main/java/com/dabsquared/gitlabjenkins/cause/CauseData.java # src/main/java/com/dabsquared/gitlabjenkins/trigger/handler/merge/MergeRequestHookTriggerHandlerLegacyImpl.java

Fixed issue with Commit Status Updater where it would crash if the bu…

3837e63

…ild was initiated manually instead of through a GitLab trigger.

Merge branch 'master' into merge-requests-1.2

3f08ce5

omehegan reviewed May 19, 2016
View reviewed changes

- Merged master from upstream.

692a9c3

- Addressed comments in PR

omehegan reviewed May 19, 2016
View reviewed changes

omehegan added the verification-needed label May 25, 2016

Merged latest changes in master

a420f6a

McFoggy mentioned this pull request Jun 1, 2016

Missing build status for MR from forks #344

Closed

coder-hugo reviewed Jun 5, 2016
View reviewed changes

pbendersky added 2 commits June 6, 2016 16:29

- Updated Readme to reflect that Modern Mode doesn't support re-build…

112fcc9

…ing open MR when pushing to source branch. - Changed the settings so when switching the plugin Mode the options for "triggerOpenMergeRequest" are refreshed and show only the possible values.

Merge branch 'master' into merge-requests-1.2

59ed129

Merge branch 'master' into merge-requests-1.2

697e991

Changed target to source

27c4225

pbendersky added 3 commits June 22, 2016 13:53

Merge branch 'master' into merge-requests-1.2

8b99102

fixed issues after merging

a2ff6e4

pbendersky mentioned this pull request Jun 30, 2016

Updated readme with instructions to fetch MRs from origin. #370

Closed

pbendersky closed this Sep 16, 2016

exceed-alae pushed a commit to exceed-alae/gitlab-plugin that referenced this pull request May 20, 2022

Merge pull request jenkinsci#285 from jenkinsci/master

85eec74

Merge in latest from jenkinsci

exceed-alae pushed a commit to exceed-alae/gitlab-plugin that referenced this pull request May 20, 2022

Merge pull request jenkinsci#115 from janinko/master

9974a6b

Merge pull request jenkinsci#285 from jenkinsci/master

Adds a Modern Mode handling for merge requests. #285

Adds a Modern Mode handling for merge requests. #285

Conversation

pbendersky commented May 2, 2016

pbendersky commented May 2, 2016

omehegan commented May 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pbendersky commented May 4, 2016

omehegan commented May 5, 2016

omehegan commented May 13, 2016

pbendersky commented May 13, 2016

omehegan commented May 14, 2016

tommyminds commented May 17, 2016

omehegan commented May 17, 2016

omehegan commented May 19, 2016

Choose a reason for hiding this comment

omehegan commented May 19, 2016

pbendersky commented May 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omehegan commented Jun 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pbendersky commented Jun 6, 2016

coder-hugo commented Jun 7, 2016

pbendersky commented Jun 7, 2016

pbendersky commented Jun 7, 2016

omorillo commented Jun 7, 2016

pbendersky commented Jun 8, 2016

coder-hugo commented Jun 15, 2016

pbendersky commented Jun 15, 2016

coder-hugo commented Jun 15, 2016

pbendersky commented Jun 30, 2016

pabloa commented Aug 3, 2016

pbendersky commented Aug 3, 2016

omehegan commented Sep 15, 2016

pbendersky commented Sep 16, 2016

pbendersky commented May 19, 2016 •

edited

Loading