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

[JENKINS-34547] Switch WorkflowJob.concurrentBuild to JobProperty #8

Merged
merged 7 commits into from Jul 13, 2016

Conversation

Projects
None yet
3 participants
@abayer
Copy link
Member

abayer commented Jul 12, 2016

JENKINS-34547

cc @reviewbybees most especially @jglick

@abayer

This comment has been minimized.

Copy link
Member

abayer commented Jul 12, 2016

Hmm - looks like JobPropertyStep may need to be moved into here for test purposes, at least.

@abayer

This comment has been minimized.

Copy link
Member

abayer commented Jul 12, 2016

Or the test needs to go in workflow-multibranch-plugin.git...

}

@Extension
@Symbol("concurrentBuildProperty")

This comment has been minimized.

@jglick

jglick Jul 12, 2016

Member

🐛 do not include property in the symbol name. It is implicit in the extension point.

This comment has been minimized.

@abayer

abayer Jul 12, 2016

Member

Okiedokie - I was wary of just going with "concurrentBuild" since this is Pipeline-specific...

* {@link WorkflowJobProperty} for setting whether a job should allow concurrent builds.
*/
@ExportedBean
public class WorkflowConcurrentBuildJobProperty extends WorkflowJobProperty {

This comment has been minimized.

@jglick

jglick Jul 12, 2016

Member

🐜 just extend JobProperty.

This comment has been minimized.

@abayer

abayer Jul 12, 2016

Member

Gonna go with the OptionalJobProperty approach, I think.

return allowConcurrentBuilds;
}

synchronized void setAllowConcurrentBuilds(boolean b) {

This comment has been minimized.

@jglick

jglick Jul 12, 2016

Member

🐜 unused, delete

This comment has been minimized.

@abayer
@ExportedBean
public class WorkflowConcurrentBuildJobProperty extends WorkflowJobProperty {
/** Defaults to true */
private @CheckForNull Boolean allowConcurrentBuilds;

This comment has been minimized.

@jglick

jglick Jul 12, 2016

Member

🐛 use boolean. The absence of this property can be construed as an implicit true.

This comment has been minimized.

@jglick

jglick Jul 12, 2016

Member

Or, if you want to remove this property when allowing concurrent builds, then just delete the field altogether, and extend OptionalJobProperty.

This comment has been minimized.

@jglick

jglick Jul 12, 2016

Member

Or conversely, you could have the presence of this property mean to allow concurrent builds, the absense to not.

This comment has been minimized.

@abayer

abayer Jul 12, 2016

Member

The current default case is to allow concurrent builds, so I think we'd want to go with OptionalJobProperty...

@@ -106,7 +108,7 @@
@SuppressWarnings("deprecation")
private hudson.model.BuildAuthorizationToken authToken;
private transient LazyBuildMixIn<WorkflowJob,WorkflowRun> buildMixIn;
/** defaults to true */
/** @deprecated replaced by {@link WorkflowConcurrentBuildJobProperty} defaults to true */

This comment has been minimized.

@jglick

jglick Jul 12, 2016

Member

delete the original Javadoc

This comment has been minimized.

@abayer

abayer Jul 12, 2016

Member

Will do!

}

public void setConcurrentBuild(boolean b) throws IOException {
concurrentBuild = b ? null : false;

This comment has been minimized.

@jglick

jglick Jul 12, 2016

Member

I think you now need to unconditionally set concurrentBuild to null. And deprecate this method.

Either way, you should start off by writing the @LocalData-based tests which define what you expect to happen w.r.t. settings compatibility.

This comment has been minimized.

@abayer

abayer Jul 12, 2016

Member

Ok, will do.

abayer added some commits Jul 12, 2016

@@ -138,6 +140,9 @@ public WorkflowJob(ItemGroup parent, String name) {
for (Trigger t : triggers) {
t.start(this, Items.currentlyUpdatingByXml());
}
if (concurrentBuild != null) {

This comment has been minimized.

@abayer

abayer Jul 12, 2016

Member

Not happy about this, but setConcurrentBuild(boolean) alone wasn't enough to get the migration working.

@jglick

This comment has been minimized.

Copy link
Member

jglick commented Jul 12, 2016

Need to use configRoundTrip to see that the property can be saved & redisplayed on a non-multibranch project.

@abayer

This comment has been minimized.

Copy link
Member

abayer commented Jul 12, 2016

@jglick - clarifying - you mean just a JenkinsRule.configRoundTrip(job)?

@abayer

This comment has been minimized.

Copy link
Member

abayer commented Jul 13, 2016

So the OptionalJobProperty approach has actually broken this logic pretty hard - need to do some reworking.

@abayer

This comment has been minimized.

Copy link
Member

abayer commented Jul 13, 2016

There's a nasty little contradiction - if we want just a checkbox in the UI, we need OptionalJobProperty, but if we want to have concurrent builds enabled by default with a way to disable them from the properties step, OptionalJobProperty doesn't work. Grr.

Reworked to an OptionalJobProperty to disable concurrent builds.
Having an OptionalJobProperty used solely as a boolean, effectively,
means we need to have the default case be in effect when the property
doesn't exist. That's different than the traditional concurrent build
check, where the default is to not allow concurrent builds. Since we
do want concurrent builds by default in Pipeline jobs, we need to
instead have the property be for *disabling* concurrent
builds. So...yeah.

Not sure yet how to do a UI config test for an OptionalJobProperty -
need to work on that.
@jglick

This comment has been minimized.

Copy link
Member

jglick commented Jul 13, 2016

you mean just a JenkinsRule.configRoundTrip(job)

Yes.

if we want to have concurrent builds enabled by default

No strong opinion. Originally this was the only option for Pipeline. But for freestyle projects, the default has always been nonconcurrent, with the option for concurrent added later. Certainly we need to preserve the behavior of existing jobs.

with a way to disable them from the properties step, OptionalJobProperty doesn't work

Why is that?

@@ -138,6 +140,9 @@ public WorkflowJob(ItemGroup parent, String name) {
for (Trigger t : triggers) {
t.start(this, Items.currentlyUpdatingByXml());
}
if (concurrentBuild != null && !concurrentBuild) {

This comment has been minimized.

@jglick

jglick Jul 13, 2016

Member

🐜 Do you not want to call setConcurrentBuild even if it is false, just in case? Otherwise a true value will stick around in config.xml indefinitely. We would like to unconditionally delete the deprecated field.

This comment has been minimized.

@abayer

abayer Jul 13, 2016

Member

Yeah, you're right there. That was an artifact of earlier efforts. Fixing!

return false;
} else {
/* settings compatibility */
return !Boolean.FALSE.equals(concurrentBuild);

This comment has been minimized.

@jglick

jglick Jul 13, 2016

Member

🐜 Can just become

return true;

now, since onLoad would already have cleared the field.

This comment has been minimized.

@abayer

abayer Jul 13, 2016

Member

Even simpler -

return getProperty(DisableConcurrentBuildsJobProperty.class) == null;

=)

try {
removeProperty(DisableConcurrentBuildsJobProperty.class);
if (!b) {
addProperty(new DisableConcurrentBuildsJobProperty());

This comment has been minimized.

@jglick

jglick Jul 13, 2016

Member

Could be optimized to skip a save in case the setting does not change.

This comment has been minimized.

@abayer

abayer Jul 13, 2016

Member

Adding such an optimization with a few line comment explaining what exactly the logic is for the optimization, since it's a bit weird at first glance. =)

@@ -276,4 +277,21 @@ public void failedToStartRun() throws Exception {
assertEquals(Collections.emptyList(), iba.getCauses());
}

@Issue("JENKINS-34547")
@LocalData
@Test public void concurrentBuildsMigrationOnByDefault() throws Exception {

This comment has been minimized.

@jglick

jglick Jul 13, 2016

Member

🐜 the code in question is in WorkflowJob, so it would make more sense for this to be in a WorkflowJobTest. Or in a DisableConcurrentBuildsJobPropertyTest.

This comment has been minimized.

@abayer

abayer Jul 13, 2016

Member

Yup, moved around.

@jglick

This comment has been minimized.

Copy link
Member

jglick commented Jul 13, 2016

🐝

@jglick

This comment has been minimized.

Copy link
Member

jglick commented Jul 13, 2016

if we want to have concurrent builds enabled by default
No strong opinion.

Though I tend to prefer concurrent by default, since to my way of thinking the serial option is a special behavior, akin to a QueueTaskDispatcher which blocks builds from running under some conditions (example)—in this case, just that another build of the same job is currently running.

@abayer

This comment has been minimized.

Copy link
Member

abayer commented Jul 13, 2016

So given that the default behavior has been to allow concurrent builds, I think that has to stay the same. Therefore, switching the UI from a checked-by-default checkbox for enabling concurrent builds to an unchecked-by-default checkbox for disabling concurrent builds seems eminently reasonable for me.

@jglick

This comment has been minimized.

Copy link
Member

jglick commented Jul 13, 2016

given that the default behavior has been to allow concurrent builds, I think that has to stay the same

Well…not for jobs saved since the concurrentBuild field was added. But for really old jobs last saved before there was even the option, yes.

switching the UI from a checked-by-default checkbox for enabling concurrent builds to an unchecked-by-default checkbox for disabling concurrent builds seems eminently reasonable for me

I am not opposed to that necessarily, though users might be confused that the UI is opposite that of freestyle projects. If we want to keep a consistent UI then we cannot use OptionalJobProperty, I am afraid.

if (!b) {
addProperty(new DisableConcurrentBuildsJobProperty());

boolean propertyExists = getProperty(DisableConcurrentBuildsJobProperty.class) == null;

This comment has been minimized.

@jglick

jglick Jul 13, 2016

Member

🐜 the variable name is the opposite of the meaning.

This comment has been minimized.

@abayer

abayer Jul 13, 2016

Member

Also the comment was the inverse of what was actually being done. Whoops!

@abayer

This comment has been minimized.

Copy link
Member

abayer commented Jul 13, 2016

Yeah, the UI inconsistency is the only way I see to have just a checkbox with no further config in the UI, where it can also be disabled via the properties step. Not to mention there's a challenge in getting an OptionalJobProperty to be selected by default!

@jglick

This comment has been minimized.

Copy link
Member

jglick commented Jul 13, 2016

the UI inconsistency is the only way I see to have just a checkbox with no further config in the UI, where it can also be disabled via the properties step

Well the alternative is a non-Optional JobProperty whose config view has a checkbox in it (checked by default), and from Jenkinsfile you would need to write something like

properties([concurrentBuilds(false)])

In some ways this is a little cleaner since it is apparent which jobs have been updated to the new format: all newly saved jobs should have the property.

@jglick

This comment has been minimized.

Copy link
Member

jglick commented Jul 13, 2016

re🐝

@abayer

This comment has been minimized.

Copy link
Member

abayer commented Jul 13, 2016

Yeah, I considered the traditional JobProperty approach but dislike that UX more than changing the behavior.

}
bc.commit();
} finally {
bc.abort();

This comment has been minimized.

@svanoort

svanoort Jul 13, 2016

Member

Why are we aborting after committing when successful?

This comment has been minimized.

@abayer

abayer Jul 13, 2016

Member

That was lifted from somewhere - lemme find it. I think that the abort ends up doing nothing post-commit....

This comment has been minimized.

@abayer

abayer Jul 13, 2016

Member

Ah, this was the paradigm used before jenkinsci/jenkins@5b4dbb3, and since we're not basing on HEAD of core, we still need to do things the old way.

This comment has been minimized.

@svanoort

svanoort Jul 13, 2016

Member

I mean yeah it shouldn't, but it's odd to me to do it there rather than an abort and re-throw on error.

This comment has been minimized.

@abayer

abayer Jul 13, 2016

Member

I'm assuming that the paradigm is saying "Eh, if it goes wrong, just don't do it and move on"?

This comment has been minimized.

@abayer

abayer Jul 13, 2016

Member

shrug

This comment has been minimized.

@abayer

abayer Jul 13, 2016

Member

I'd say the real issue isn't the code path - it's the name abort, since what it really should be is something like cleanup since it only aborts if the change is incomplete. Anyway, after that commit I linked above, it's irrelevant.

This comment has been minimized.

@svanoort

svanoort Jul 13, 2016

Member

Although... the alternative is pretty ugly (catch Exception, try to abort, then finally rethrow exception?).

This comment has been minimized.

@svanoort

svanoort Jul 13, 2016

Member

Meh. 🐝 and some grumping about the BulkChange APIs from me.

This comment has been minimized.

@jglick

jglick Jul 13, 2016

Member

The code is correct here.

@abayer

This comment has been minimized.

Copy link
Member

abayer commented Jul 13, 2016

Yay, @reviewbybees done

@jglick jglick merged commit d2962d1 into jenkinsci:master Jul 13, 2016

1 check passed

Jenkins This pull request looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment