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

[JENKINS-27299] Define disabled in ParameterizedJob rather than AbstractProject #2866

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
2e27be7
Offering default methods on ParameterizedJob.
jglick May 1, 2017
fbadce3
Javadoc typo.
jglick May 1, 2017
626b876
Cleaner use of default methods in ParameterizedJob.
jglick May 2, 2017
3139f23
Need to pick up https://github.com/infradna/bridge-method-injector/pu…
jglick May 2, 2017
d71db0f
Sketch of pulling disabled functionality into ParameterizedJob.
jglick May 3, 2017
9318eef
EnableJobCommandTest.groovy → EnableJobCommandTest.java, and replacin…
jglick May 4, 2017
b7e1d86
All CLI commands could be broken by a missing CLI.*.shortDescription …
jglick May 4, 2017
41630e2
Forgot to move CLI method short descriptions to new package.
jglick May 4, 2017
1fffbed
Needed a @CLIResolver for ParameterizedJob. Adding an OptionHandler w…
jglick May 4, 2017
cd95d2b
Trying to fix up access-modifier versions; started failing in CI toda…
jglick May 4, 2017
137b8ec
Introduced <p:makeDisabled/> by analogy with <p:config-disableBuild/>.
jglick May 4, 2017
36a5301
Using new type bounds.
jglick May 4, 2017
1c759d3
Merge branch 'ParameterizedJobMixIn-defaults' into AbstractProject.di…
jglick May 4, 2017
59fb6e6
access-modifier 1.11 released.
jglick May 4, 2017
aeb9952
MatrixProject and MavenModuleSet both expect to have access to makeDi…
jglick May 4, 2017
bb617a6
Trying to generalize some more.
jglick May 4, 2017
1517179
Minor simplification.
jglick May 4, 2017
265ae0a
[JENKINS-34716] Generalizing doPolling and schedulePolling.
jglick May 4, 2017
b6459e1
isBuildable
jglick May 5, 2017
30c6674
Obsolete comment.
jglick May 5, 2017
244e419
Updated comments.
jglick May 5, 2017
82ad44a
bridge-method-injector 1.17
jglick May 5, 2017
d24bb16
Unfortunately AbstractProject.schedulePolling cannot delegate to SCMT…
jglick May 5, 2017
faaee4c
bridge-method-injector 1.17
jglick May 9, 2017
8f49071
Merge branch 'master' into ParameterizedJobMixIn-defaults
jglick May 9, 2017
0a00506
Merge branch 'ParameterizedJobMixIn-defaults' into AbstractProject.di…
jglick May 9, 2017
f0b79a0
Merge branch 'master' into AbstractProject.disabled-JENKINS-27299
jglick May 10, 2017
500037a
Merge branch 'master' into AbstractProject.disabled-JENKINS-27299
oleg-nenashev May 13, 2017
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion cli/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
<dependency>
<groupId>org.kohsuke</groupId>
<artifactId>access-modifier-annotation</artifactId>
<version>1.7</version>
</dependency>
<dependency>
<groupId>commons-codec</groupId>
Expand Down
2 changes: 2 additions & 0 deletions core/move-l10n.groovy
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ for (p in new File(resDir, oldview).parentFile.listFiles()) {
def n = p.name;
if (n == "${basename}.properties" || n.startsWith("${basename}_") && n.endsWith(".properties")) {
def lines = p.readLines('ISO-8859-1');
// TODO does not handle multiline values correctly
def matches = lines.findAll({it.startsWith("${key}=")});
if (!matches.isEmpty()) {
lines.removeAll(matches);
Expand All @@ -24,6 +25,7 @@ for (p in new File(resDir, oldview).parentFile.listFiles()) {
} else {
def nue = new File(resDir, newview + n.substring(basename.length()));
println("moving ${matches.size()} matches from ${n} to ${nue.name}");
// TODO if the original lacked a trailing newline, this will corrupt previously final key
nue.withWriterAppend('ISO-8859-1') {out ->
matches.each {line -> out.writeLine(line)}
}
Expand Down
2 changes: 0 additions & 2 deletions core/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -575,7 +575,6 @@ THE SOFTWARE.
<dependency>
<groupId>org.kohsuke</groupId>
<artifactId>access-modifier-annotation</artifactId>
<version>1.4</version>
</dependency>

<dependency>
Expand Down Expand Up @@ -704,7 +703,6 @@ THE SOFTWARE.
<plugin>
<groupId>org.kohsuke</groupId>
<artifactId>access-modifier-checker</artifactId>
<!-- version specified in grandparent pom -->
<executions>
<execution>
<goals>
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/hudson/cli/BuildCommand.java
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ protected int run() throws Exception {

if (!job.isBuildable()) {
String msg = Messages.BuildCommand_CLICause_CannotBuildUnknownReasons(job.getFullDisplayName());
if (job instanceof AbstractProject<?, ?> && ((AbstractProject<?, ?>)job).isDisabled()) {
if (job instanceof ParameterizedJobMixIn.ParameterizedJob && ((ParameterizedJobMixIn.ParameterizedJob) job).isDisabled()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not 100% clear. Would be preferable to also have it on the Job level directly or in BuildableItem. But better than it was before

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is indeed a little tricky. Job does define buildable (without any explanation), but the idea here was to define disabled in ParameterizedJob since that is now the home for “Job which we could potentially request a build of, perhaps with parameters, so long as that is not disabled” (i.e., basically everything except ExternalJob).

BuildableItem (not to be confused with Queue.BuildableItem!), well, this interface is a historical relic: very little code actually deals with objects declared to be of this type and expects scheduleBuild overloads to work. I am implementing it for completeness; AbstractProject and WorkflowJob, the only ParameterizedJobs out there, already implemented it anyway, so this is just to make it easier to provide default implementations for those four methods (mostly superseded by scheduleBuild2 overloads).

msg = Messages.BuildCommand_CLICause_CannotBuildDisabled(job.getFullDisplayName());
} else if (job.isHoldOffBuildUntilSave()){
msg = Messages.BuildCommand_CLICause_CannotBuildConfigNotSaved(job.getFullDisplayName());
Expand Down
3 changes: 2 additions & 1 deletion core/src/main/java/hudson/cli/declarative/CLIRegisterer.java
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@
import java.util.Collections;
import java.util.List;
import java.util.Locale;
import java.util.MissingResourceException;
import java.util.Stack;
import static java.util.logging.Level.SEVERE;

Expand Down Expand Up @@ -279,7 +280,7 @@ protected int run() throws Exception {
throw new UnsupportedOperationException();
}
}));
} catch (ClassNotFoundException e) {
} catch (ClassNotFoundException | MissingResourceException e) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

huh? is it related?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, see commit introducing it. The enable-job and disable-job @CLIMethods were moved, and if you forget to move the corresponding shortDescription, as I initially did, you not only break those CLI commands, you break every other CLI command too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed

LOGGER.log(SEVERE,"Failed to process @CLIMethod: "+m,e);
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
/*
* The MIT License
*
* Copyright 2017 CloudBees, Inc.
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
* in the Software without restriction, including without limitation the rights
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
* copies of the Software, and to permit persons to whom the Software is
* furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in
* all copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
* THE SOFTWARE.
*/

package hudson.cli.handlers;

import jenkins.model.ParameterizedJobMixIn;
import org.kohsuke.MetaInfServices;
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.DoNotUse;
import org.kohsuke.args4j.CmdLineParser;
import org.kohsuke.args4j.OptionDef;
import org.kohsuke.args4j.spi.OptionHandler;
import org.kohsuke.args4j.spi.Setter;

/**
* Refer to {@link jenkins.model.ParameterizedJobMixIn.ParameterizedJob} by its name.
*/
@Restricted(DoNotUse.class)
@MetaInfServices(OptionHandler.class)
@SuppressWarnings("rawtypes")
public class ParameterizedJobOptionHandler extends GenericItemOptionHandler<ParameterizedJobMixIn.ParameterizedJob> {

public ParameterizedJobOptionHandler(CmdLineParser parser, OptionDef option, Setter<ParameterizedJobMixIn.ParameterizedJob> setter) {
super(parser, option, setter);
}

@Override
protected Class<ParameterizedJobMixIn.ParameterizedJob> type() {
return ParameterizedJobMixIn.ParameterizedJob.class;
}

@Override
public String getDefaultMetaVariable() {
return "JOB";
}

}
2 changes: 1 addition & 1 deletion core/src/main/java/hudson/model/AbstractItem.java
Original file line number Diff line number Diff line change
Expand Up @@ -854,7 +854,7 @@ public static AbstractItem resolveForCLI(
// TODO can this (and its pseudo-override in AbstractProject) share code with GenericItemOptionHandler, used for explicit CLICommand’s rather than CLIMethod’s?
AbstractItem item = Jenkins.getInstance().getItemByFullName(name, AbstractItem.class);
if (item==null) {
AbstractProject project = AbstractProject.findNearest(name);
AbstractProject project = AbstractProject.findNearest(name); // TODO should be Items.findNearest
throw new CmdLineException(null, project == null ? Messages.AbstractItem_NoSuchJobExistsWithoutSuggestion(name)
: Messages.AbstractItem_NoSuchJobExists(name, project.getFullName()));
}
Expand Down
59 changes: 16 additions & 43 deletions core/src/main/java/hudson/model/AbstractProject.java
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@
import hudson.Functions;
import hudson.Launcher;
import hudson.Util;
import hudson.cli.declarative.CLIMethod;
import hudson.cli.declarative.CLIResolver;
import hudson.model.Cause.LegacyCodeCause;
import hudson.model.Descriptor.FormException;
Expand All @@ -48,7 +47,6 @@
import hudson.model.Queue.Task;
import hudson.model.labels.LabelAtom;
import hudson.model.labels.LabelExpression;
import hudson.model.listeners.ItemListener;
import hudson.model.listeners.SCMPollListener;
import hudson.model.queue.CauseOfBlockage;
import hudson.model.queue.QueueTaskFuture;
Expand Down Expand Up @@ -110,6 +108,7 @@
import jenkins.scm.SCMCheckoutStrategy;
import jenkins.scm.SCMCheckoutStrategyDescriptor;
import jenkins.scm.SCMDecisionHandler;
import jenkins.triggers.SCMTriggerItem;
import jenkins.util.TimeDuration;
import net.sf.json.JSONObject;
import org.acegisecurity.Authentication;
Expand Down Expand Up @@ -640,7 +639,7 @@ public boolean hasCustomScmCheckoutRetryCount(){

@Override
public boolean isBuildable() {
return !isDisabled() && !isHoldOffBuildUntilSave();
return ParameterizedJobMixIn.ParameterizedJob.super.isBuildable();
}

/**
Expand Down Expand Up @@ -669,10 +668,17 @@ public void setBlockBuildWhenUpstreamBuilding(boolean b) throws IOException {
save();
}

@Override
public boolean isDisabled() {
return disabled;
}

@Restricted(DoNotUse.class)
@Override
public void setDisabled(boolean disabled) {
this.disabled = disabled;
}

/**
* Validates the retry count Regex
*/
Expand All @@ -687,38 +693,22 @@ public FormValidation doCheckRetryCount(@QueryParameter String value)throws IOEx
}

/**
* Marks the build as disabled.
* The method will ignore the disable command if {@link #supportsMakeDisabled()}
* returns false. The enable command will be executed in any case.
* @param b true - disable, false - enable
* @since 1.585 Do not disable projects if {@link #supportsMakeDisabled()} returns false
*/
public void makeDisabled(boolean b) throws IOException {
if(disabled==b) return; // noop
if (b && !supportsMakeDisabled()) return; // do nothing if the disabling is unsupported
this.disabled = b;
if(b)
Jenkins.getInstance().getQueue().cancel(this);

save();
ItemListener.fireOnUpdated(this);
}

/**
* Specifies whether this project may be disabled by the user.
* {@inheritDoc}
* By default, it can be only if this is a {@link TopLevelItem};
* would be false for matrix configurations, etc.
* @return true if the GUI should allow {@link #doDisable} and the like
* @since 1.475
*/
@Override
public boolean supportsMakeDisabled() {
return this instanceof TopLevelItem;
}

// Seems to be used only by tests; do not bother pulling up.
public void disable() throws IOException {
makeDisabled(true);
}

// Ditto.
public void enable() throws IOException {
makeDisabled(false);
}
Expand Down Expand Up @@ -866,6 +856,7 @@ public QueueTaskFuture<R> scheduleBuild2(int quietPeriod, Action... actions) {

/**
* Schedules a polling of this project.
* @see SCMTriggerItem#schedulePolling
*/
public boolean schedulePolling() {
if(isDisabled()) return false;
Expand Down Expand Up @@ -1173,6 +1164,7 @@ public List<SubTask> getSubTasks() {
return r;
}

@Override // same as ParameterizedJob version except calls possibly overridden newBuild
public @CheckForNull R createExecutable() throws IOException {
if(isDisabled()) return null;
return newBuild();
Expand Down Expand Up @@ -1742,9 +1734,7 @@ public void doBuildWithParameters(StaplerRequest req, StaplerResponse rsp) throw
doBuildWithParameters(req, rsp, TimeDuration.fromString(req.getParameter("delay")));
}

/**
* Schedules a new SCM polling command.
*/
@Override // in case schedulePolling was overridden
public void doPolling( StaplerRequest req, StaplerResponse rsp ) throws IOException, ServletException {
BuildAuthorizationToken.checkPermission((Job) this, authToken, req, rsp);
schedulePolling();
Expand Down Expand Up @@ -1888,23 +1878,6 @@ public HttpResponse doDoWipeOutWorkspace() throws IOException, ServletException,
}
}

@CLIMethod(name="disable-job")
@RequirePOST
public HttpResponse doDisable() throws IOException, ServletException {
checkPermission(CONFIGURE);
makeDisabled(true);
return new HttpRedirect(".");
}

@CLIMethod(name="enable-job")
@RequirePOST
public HttpResponse doEnable() throws IOException, ServletException {
checkPermission(CONFIGURE);
makeDisabled(false);
return new HttpRedirect(".");
}


/**
* {@link AbstractProject} subtypes should implement this base class as a descriptor.
*
Expand Down
5 changes: 3 additions & 2 deletions core/src/main/java/hudson/model/ListView.java
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
import javax.annotation.concurrent.GuardedBy;
import javax.servlet.ServletException;
import jenkins.model.Jenkins;
import jenkins.model.ParameterizedJobMixIn;

import net.sf.json.JSONObject;
import org.jenkinsci.Symbol;
Expand Down Expand Up @@ -197,8 +198,8 @@ public List<TopLevelItem> getItems() {
for (TopLevelItem item : candidates) {
if (!names.contains(item.getRelativeNameFrom(getOwnerItemGroup()))) continue;
// Add if no status filter or filter matches enabled/disabled status:
if(statusFilter == null || !(item instanceof AbstractProject)
|| ((AbstractProject)item).isDisabled() ^ statusFilter)
if(statusFilter == null || !(item instanceof ParameterizedJobMixIn.ParameterizedJob) // TODO or better to call the more generic Job.isBuildable?
|| ((ParameterizedJobMixIn.ParameterizedJob)item).isDisabled() ^ statusFilter)
items.add(item);
}

Expand Down
Loading