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

Update to filter ChangeKind in PatchsetCreatedEvents #159

Merged
merged 1 commit into from Jun 16, 2014

Conversation

Projects
None yet
8 participants
@vangdfang
Copy link
Contributor

commented May 29, 2014

ChangeKind is now exposed by Gerrit, which gives Jenkins the ability
to filter for changes that are trivial rebases or changes that do not
impact code.

Depends on sonyxperiadev/gerrit-events#17

@@ -87,6 +111,14 @@ public boolean shouldTriggerOn(GerritTriggeredEvent event) {
if (excludeDrafts && ((PatchsetCreated)event).getPatchSet().isDraft()) {
return false;
}
if (excludeTrivialRebase
&& new String("TRIVIAL_REBASE").equals(((PatchsetCreated)event).getPatchSet().getKind())) {

This comment has been minimized.

Copy link
@vangdfang

vangdfang May 29, 2014

Author Contributor

Really not a fan of hardcoding these strings (especially because of the test cases), so I'm open to a new and better way. Please feel free to suggest one. :)

This comment has been minimized.

Copy link
@rinrinne

rinrinne May 30, 2014

Member

Add a class like GerritEventKeys to gerrit-events then use it.

This comment has been minimized.

Copy link
@rsandell

rsandell Jun 3, 2014

Member

agree, se my comment on gerrit-events.

@cloudbees-pull-request-builder

This comment has been minimized.

Copy link

commented May 29, 2014

plugins » gerrit-trigger-plugin #281 FAILURE
Looks like there's a problem with this pull request

@cloudbees-pull-request-builder

This comment has been minimized.

Copy link

commented May 29, 2014

plugins » gerrit-trigger-plugin #282 FAILURE
Looks like there's a problem with this pull request

@jenkinsadmin

This comment has been minimized.

Copy link
Member

commented May 30, 2014

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

@rinrinne

This comment has been minimized.

Copy link
Member

commented May 30, 2014

You should not care anything until sonyxperiadev/gerrit-events#17 is merged then released.

And, does this change have something relationship to JENKINS-23236?

@orgads

This comment has been minimized.

Copy link
Contributor

commented May 30, 2014

What happens if a build is in progress, then a new "no code change" patchset is uploaded? The previous build will not be able to vote for the change, and there will be no subsequent build.

Is it possible to retarget the patchset for an active build on this case?

@vangdfang

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2014

@rinrinne Hah! I had no idea people were following Gerrit master that closely. It does look to be related, though, yes. :) I did submit them side-by-side so people can experiment with this for now (knowing full well the CI won't be able to build this without the related gerrit-events change).

@orgads I thought about that, and the best I could come up with is, a user could easily do the same thing by submitting a draft with the "Exclude Drafts" box of the PatchsetCreated event checked. Any scores (if Jenkins doesn't abort the build due to the "Build Current Version Only" being enabled) will be ignored.

The other thing I was thinking about, it would be nice if there were a version check for Gerrit 2.10 before displaying those checkboxes to avoid confusion. I considered adding it, but I'm not entirely sure how with the Jelly markup.

@jkugler

This comment has been minimized.

Copy link

commented May 30, 2014

@orgads I might not be understanding correctly, but if a build is in progress on change,patch #XXXX,Y when it finishes, it will report its status, regardless of whether or not a new change #XXXX,Z has been submitted in the mean time, no?

@jhansche

This comment has been minimized.

Copy link
Member

commented May 30, 2014

@jkugler The problem is that the build in progress (B1) on #XXXX,Y will either a) abort because of "Build Current Version Only", or b) continue to build and finish with SUCCESS or FAILURE. But because #XXXX,Z has already been uploaded, B1 will not be able to vote on #XXXX,Y, because it is no longer the current patchset (does not accept approval scores).

If B1 aborted because of (a), then there also will be no build for #XXXX,Z due to TRIVIAL_REBASE, thus no approval score will get reported. If B1 did not abort, then it will report its status as a comment, but it will not be able to apply an approval score.

In the case of NO_CODE_CHANGE, it could reassign the active build to the new patchset, so that its resulting vote will be applied to the #XXXX,Z patchset instead of to ,Y. But then you run into a problem if the job itself is supposed to check something like the commit message (which may have changed).

I think the best approach would be to allow the new change to be rebuilt even on TRIVIAL_REBASE, iff the build for the previous patchset ended in ABORTED status.

@jkugler

This comment has been minimized.

Copy link

commented May 30, 2014

@jhansche Ah, I see what you mean. I think allowing to build an ABORTED build would also apply for NO_CODE_CHANGE, yes?

@jhansche

This comment has been minimized.

Copy link
Member

commented May 30, 2014

I would think so too -- ABORTED means the previous patchset didn't actually build, so if you have a new opportunity to build now, I think it makes sense to allow the build to continue. Not sure how easy it is to determine which build was for the previous patchset (there might not even be a build for the previous patchset, if it was canceled before it started).

@@ -26,4 +26,12 @@
field="excludeDrafts">
<f:checkbox/>
</f:entry>
<f:entry title="${%Exclude Trivial Rebase}"
field="excludeTrivialRebase">

This comment has been minimized.

Copy link
@rsandell

rsandell Jun 3, 2014

Member

I think some help files would be god to have here.

This comment has been minimized.

Copy link
@vangdfang

vangdfang Jun 5, 2014

Author Contributor

Done. Will be coming once my test build passes. :)

@cloudbees-pull-request-builder

This comment has been minimized.

Copy link

commented Jun 5, 2014

plugins » gerrit-trigger-plugin #284 FAILURE
Looks like there's a problem with this pull request

@cloudbees-pull-request-builder

This comment has been minimized.

Copy link

commented Jun 5, 2014

plugins » gerrit-trigger-plugin #285 FAILURE
Looks like there's a problem with this pull request

Update to filter ChangeKind in PatchsetCreatedEvents
ChangeKind is now exposed by Gerrit, which gives Jenkins the ability
to filter for changes that are trivial rebases or changes that do not
impact code.
@cloudbees-pull-request-builder

This comment has been minimized.

Copy link

commented Jun 16, 2014

plugins » gerrit-trigger-plugin #289 SUCCESS
This pull request looks good

rsandell added a commit that referenced this pull request Jun 16, 2014

Merge pull request #159 from vangdfang/master
Update to filter ChangeKind in PatchsetCreatedEvents

@rsandell rsandell merged commit 21df17b into jenkinsci:master Jun 16, 2014

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.