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

Fix for "Build Current Patches Only" #215

Merged
merged 5 commits into from Apr 27, 2015

Conversation

Projects
None yet
6 participants
@TWestling
Copy link
Member

commented Mar 19, 2015

The feature "Build Current Patches Only" cancels
builds that are already running for the same
change. I've added checks for if one of the triggers
was manual trigger as well as if an older patch set
gets triggered (due to a retrigger),
where the builds won't get cancelled.

TWestling added some commits Feb 20, 2015

Fixes for "Build Current Patches Only"
The feature "Build Current Patches Only" cancels
builds that are already running for the same
change. I've added checks for if one of the triggers
was manual trigger as well as if an older patch set
gets triggered (due to a retrigger),
where the builds won't get cancelled.

Change-Id: I4131f0a7f305df39d2fd21ea3f745a619e5d83c3
@scoheb

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2015

Hi,

I believe both of the new conditions that you have introduced should be added a configurable options. We have internal customers how would like to retain the current behavior.

Thanks

Scott

@jenkinsadmin

This comment has been minimized.

Copy link
Member

commented Mar 19, 2015

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

if (!(runningChangeBasedEvent instanceof ManualPatchsetCreated)
&& runningChangeBasedEvent.getChange().equals(event.getChange())
&& Integer.parseInt(runningChangeBasedEvent.getPatchSet().getNumber())
< Integer.parseInt(event.getPatchSet().getNumber())) {

This comment has been minimized.

Copy link
@rsandell

rsandell Mar 20, 2015

Member

having runningChangeBasedEvent.getPatchSet().compareTo(event.getPatchSet()) < 0 or something along those lines would have been nicer.

@rsandell

This comment has been minimized.

Copy link
Member

commented Mar 20, 2015

I was debating this (at least with myself) when the new behaviour change got introduced in a pull request. But couldn't motivate it good enough in my head. But being able to configure weather to ignore retriggers and manual triggered seems like a good thing since different people prefer different behaviours.

@TWestling

This comment has been minimized.

Copy link
Member Author

commented Mar 20, 2015

I will upload a new change with configuration for this (as well as improve the comparison between new and old patchsets).

@oharboe

This comment has been minimized.

Copy link
Contributor

commented Mar 23, 2015

We're currently struggling with the manual rebuild, because it doesn't retrigger a build for trivial rebase patchsets.

To trigger builds for trivial rebase patchsets by default swamps our slaves with unnecessary work, because developers frequently have a string of reviews where they only work on the latest review, but they need to rebase all the reviews on top of the newest server branch.

Skipping rebuilds on trivial rebase is problematic when there is a failure on the server branch that interacts with a review. If there is a false positive or negative, then the developer does want a rebuild even if the rebase was trivial.

#216 makes it easy for developers to manually request a build in this case because #216 will always trigger a build on manual rebuild, even if the rebase was a trivial rebase. I don't want to have a configuration or GUI option here if we can avoid it. It seems like clutter.

I mention this because this pull-request is somewhat related to #216.

Thoughts?

@xbeta

This comment has been minimized.

Copy link

commented Mar 26, 2015

I don't even understand why we can't override this behavior in manual trigger. It's manual so that means we know we want to force it to build it no matter of what. Am I missing something here? I would like to join this discussion as to why this behavior has changed.

@oharboe

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2015

What changed is that a new feature has been added: that builds can be
skipped for trivial rebase, comment only and draft reviews.

IMO, manual trigger should continue to do what it has always done, even
though the new skip build feature has been added: always trigger a build.

There are ways around it using new voting labels, topics, labels and
detailed instructions to developers, but they already know how to use
manual build and I don't particularly want to make things any more
complicated than necessary.

On Thu, Mar 26, 2015 at 8:41 PM, Sam Xiao notifications@github.com wrote:

I don't even understand why we can't override this behavior in manual
trigger. It's manual so that means we know we want to force it to build it
no matter of what. Am I missing something here? I would like to join this
discussion as to why this behavior has changed.


Reply to this email directly or view it on GitHub
#215 (comment)
.

Øyvind Harboe - Can Zylin Consulting help on your project?
http://www.zylin.com/

@xbeta

This comment has been minimized.

Copy link

commented Mar 27, 2015

+1 with @oharboe

@oharboe

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2015

Look at #216, it's a focused pullrequest to fix the problem with manual
retrigger not retriggering builds.

#215 handles other aspects of manual retrigger and canceling builds that
are already started.

On Fri, Mar 27, 2015 at 6:07 AM, Sam Xiao notifications@github.com wrote:

+1 with @oharboe https://github.com/oharboe


Reply to this email directly or view it on GitHub
#215 (comment)
.

Øyvind Harboe - Can Zylin Consulting help on your project?
http://www.zylin.com/

@TWestling

This comment has been minimized.

Copy link
Member Author

commented Mar 30, 2015

This doesn't change whether a manual trigger will build, only if it will cancel other running builds( and be cancelled when another patch set is triggered ).

Regarding having this as an option, I agree that it feels like clutter( the reason I didn't make it optional in the first place ) but I didn't know that people wanted the behavior the way it was ( old patch sets cancelling new ones and manual patch sets cancelling anything ).

I feel like it's safer to add an option under advanced than to force the behavior.

Made fix for "Build Current Patches Only" optional
The fix, where Manual patch sets and old patch sets didn't
abort builds of newer patch sets has been made optional,
with 2 checkboxes in the advanced part of the server configuration.

Change-Id: I3af5b212bdf0817e627d201f026d9256c74d65b9
@TWestling

This comment has been minimized.

Copy link
Member Author

commented Apr 1, 2015

I can't reproduce the failed test locally.

@@ -149,6 +158,8 @@
private boolean restCodeReview;
private boolean restVerified;
private boolean gerritBuildCurrentPatchesOnly;
private boolean gerritAbortNewPatchsets;

This comment has been minimized.

Copy link
@rsandell

rsandell Apr 8, 2015

Member

When upgrading from a previous version these fields will become false, is that the intended behaviour?

@@ -264,6 +264,18 @@
</table>
</f:repeatable>
</f:entry>
<f:entry title="${%Abort new patch sets}"

This comment has been minimized.

Copy link
@rsandell

rsandell Apr 8, 2015

Member

Build Current Patches Only should be changed to an optionalBlock with these values hidden behind.

This comment has been minimized.

Copy link
@TWestling

TWestling Apr 8, 2015

Author Member

I tried to keep this change small but I agree, it is the better way to solve this. I'll change it.

@@ -149,6 +158,8 @@
private boolean restCodeReview;
private boolean restVerified;
private boolean gerritBuildCurrentPatchesOnly;

This comment has been minimized.

Copy link
@rsandell

rsandell Apr 8, 2015

Member

A slightly better approach, to slightly lesser the amount of fields in this class would be to deprecate gerritBuildCurrentPatchesOnly, make it transient and replace it with an object containing AbortNewPatchsets and AbortManualPatchsets. It would also make the optionalBlock (commented below) simpler to implement I believe.

@LocalData
public void testBuildLatestPatchsetOnly() throws Exception {
GerritServer gerritServer = PluginImpl.getInstance().getServer(PluginImpl.DEFAULT_SERVER_NAME);
((Config)gerritServer.getConfig()).setGerritBuildCurrentPatchesOnly(true);
((Config)gerritServer.getConfig()).setGerritAbortManualPatchsets(false);

This comment has been minimized.

Copy link
@rsandell

rsandell Apr 8, 2015

Member

there are four different states to test here;

  • both false
  • both true
  • one false and the other true
@rsandell

This comment has been minimized.

Copy link
Member

commented Apr 8, 2015

Since people seem to expect different behaviours from the cancelling of builds I tend to lean on the "provide options with sensible defaults" side of the fence. So that makes me unsure on how to handle #216.
While this pr seems like a sensible approach. And it seems to do the same as #217

@oharboe

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2015

Can we separate concerns to make progress here?

I think #216 is ready to be merged as-is. Nobody will be affected by #216 unless they take explicit user-action and #216 implements the default expected behaviour: always trigger a build upon a manually triggered build.

Personally, I think we subtract from the gerrit-trigger plugin by adding options for #216, but if someone really needs it, then I think it can be addressed afterwards as a separate concern.

Made buildCurrentPatchesOnly an optionalblock and added tests.
Changed the gerritBuildCurrentPatchesOnly checkbox to an
optionalblock with the extra configuration for manual and new
patch sets hidden in the optionalblock.

Added tests for the different configuration options.

Change-Id: If99f8a4f5cd71dc01c193256214846a225ac632e
@TWestling

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2015

ping @rsandell do you want to take another look?

@@ -1846,15 +1848,26 @@ public String getHelpFile() {
*/
public synchronized void scheduled(ChangeBasedEvent event, ParametersAction parameters, String projectName) {
IGerritHudsonTriggerConfig serverConfig = getServerConfig(event);
if (serverConfig != null && !serverConfig.isGerritBuildCurrentPatchesOnly()) {
BuildCancellationPolicy buildCurrentPatchesOnly = serverConfig.getBuildCurrentPatchesOnly();

This comment has been minimized.

Copy link
@rsandell

rsandell Apr 23, 2015

Member

possible NPE when serverConfig == null

This comment has been minimized.

Copy link
@TWestling

TWestling Apr 23, 2015

Author Member

Good catch, I'll add the null check again.

@rsandell

This comment has been minimized.

Copy link
Member

commented Apr 23, 2015

@TWestling Just one, Sorry for the delay, I don't think you need me to merge once fixed ;)

Added null check
Added a null check for serverConfig.
Also made sure that the event is added to
runningJobs even if no serverConfig exists
or buildCurrentPatchesOnly is false.

Change-Id: I290c83df267cf29b6d9ba47b4758aa6b3d4bdd23
@TWestling

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2015

I did the null check but also made sure that the event is put into runningJobs even if no serverConfig exists. Otherwise, if serverConfig is changed to not be null anymore while the build is running, the build wouldn't be able to be aborted.

rsandell added a commit that referenced this pull request Apr 27, 2015

Merge pull request #215 from TWestling/master
Fix for "Build Current Patches Only"

@rsandell rsandell merged commit ddeb697 into jenkinsci:master Apr 27, 2015

1 check failed

Jenkins Looks like there's a problem with this pull request
Details
@scoheb

This comment has been minimized.

Copy link
Contributor

commented May 15, 2015

Hi,

This PR causes builds to be aborted for non-related changes.

I have fixed this in the following PR: #230

Thanks

Scott

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.