Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide GERRIT_MERGED_REVISION after change-merged event #284

Merged
merged 2 commits into from May 19, 2016

Conversation

Projects
None yet
3 participants
@polygon
Copy link

commented May 17, 2016

The newRev field of a change-merged event was not parsed yet. Since this field holds the hash of a merge commit it can be important in certain build conditions.

This patch exposes this field as a GERRIT_MERGED_REVISION variable to Jenkins builds. It requires the following PR in gerrit-events to compile successfully: sonyxperiadev/gerrit-events#55 (which is the reason why Jenkins reports a failure below)

It fixes https://issues.jenkins-ci.org/browse/JENKINS-23873

Some changes to pom.xml to update the gerrit-events version might be required. Since I'm not normally a Java person I leave that to the maintainer.

@rsandell

This comment has been minimized.

Copy link
Member

commented May 18, 2016

I've updated the pom on master.
You need to update the help file for the trigger as well that lists all the parameters.

@rsandell

This comment has been minimized.

Copy link
Member

commented May 18, 2016

Since we already have the parameter GERRIT_NEWREV for RefUpdated events, I think it's better to reuse that.

Jan Dohl added some commits May 17, 2016

Jan Dohl
Provide GERRIT_MERGED_REVISION after change-merged
The newRev field of a change-merged event was not parsed yet.
Since this field holds the hash of a merge commit it can be important
in certain build conditions.
This patch exposes this field as a GERRIT_MERGED_REVISION variable
to Jenkins builds. It requires another patch in gerrit-events to work.
Jan Dohl

@polygon polygon force-pushed the polygon:feature/newrev_cm_parsing branch from 6ed46bc to f847c10 May 18, 2016

@polygon

This comment has been minimized.

Copy link
Author

commented May 18, 2016

I think GERRIT_MERGED_REVISION is more expressive. However I also see the point of re-using GERRIT_NEWREV due to the naming in the Gerrit event stream.

I added some help text, changed the parameter name and rebased on top of the current master.

@rsandell rsandell merged commit 644d75d into jenkinsci:master May 19, 2016

1 check passed

Jenkins This pull request looks good
Details
@polygon

This comment has been minimized.

Copy link
Author

commented May 30, 2016

I hope I'm not pushing on this too much but I wonder when there is going to be a new release including this code.

@rsandell

This comment has been minimized.

Copy link
Member

commented May 31, 2016

I wanted the fix for the regression on security 170 merged before I made the next release. And that was merged and released yesterday :).

@clabu

This comment has been minimized.

Copy link

commented Apr 17, 2018

@rsandell Does this mean that if we want to build the merged result of the changeMerged() event, we need to check out the commit specified by GERRIT_NEWREV (or just take HEAD)? We currently are not, and then it looks like the implicit checkout of a Pipeline job is taking the commit of the change before it was merged. Or are we configuring our Pipeline job wrong?

@rsandell

This comment has been minimized.

Copy link
Member

commented Apr 17, 2018

@clabu Yes, if you use the gerrit build chooser ("choose what revision to build" I believe it says in the UI) it will pick the revision from the active patchset for all change based events, and for RefUpdated it picks the newRev.
So for change merged events, if you want to use the merge commit you will need a different scm configuration.

@polygon polygon deleted the polygon:feature/newrev_cm_parsing branch Apr 17, 2018

@rsandell

This comment has been minimized.

Copy link
Member

commented Apr 18, 2018

OTOH it also wouldn't be too hard to add an optional setting to GerritTriggerBuildChooser to make it pick the merge rev for change-merged instead of the generic rev. ;)

@clabu

This comment has been minimized.

Copy link

commented Apr 19, 2018

Thanks for the response @rsandell.
We did some more testing and it actually seems like the Jenkins is doing the correct thing in our case. As far as we cal tell, the build of changeMerged() event finds the latest commit on the branch and checks it out explicitly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.