Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
[JENKINS-51603] The proper way to make a parameter optional is to use…
… @DataBoundSetter.
  • Loading branch information
jglick authored and ndeloof committed Jun 5, 2018
1 parent a34f763 commit 98b3769
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 15 deletions.
28 changes: 16 additions & 12 deletions core/src/main/java/hudson/slaves/JNLPLauncher.java
Expand Up @@ -38,6 +38,7 @@
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.NoExternalUse;
import org.kohsuke.stapler.DataBoundConstructor;
import org.kohsuke.stapler.DataBoundSetter;

/**
* {@link ComputerLauncher} via JNLP.
Expand Down Expand Up @@ -68,7 +69,7 @@ public class JNLPLauncher extends ComputerLauncher {
public final String vmargs;

@Nonnull
private RemotingWorkDirSettings workDirSettings;
private RemotingWorkDirSettings workDirSettings = RemotingWorkDirSettings.getEnabledDefaults();

/**
* Constructor.
Expand All @@ -79,20 +80,18 @@ public class JNLPLauncher extends ComputerLauncher {
* will be used to enable work directories by default in new agents.
* @since 2.68
*/
@DataBoundConstructor
@Deprecated
public JNLPLauncher(@CheckForNull String tunnel, @CheckForNull String vmargs, @CheckForNull RemotingWorkDirSettings workDirSettings) {
this.tunnel = Util.fixEmptyAndTrim(tunnel);
this.vmargs = Util.fixEmptyAndTrim(vmargs);
this.workDirSettings = workDirSettings != null ? workDirSettings :
RemotingWorkDirSettings.getEnabledDefaults();
this(tunnel, vmargs);
if (workDirSettings != null) {
setWorkDirSettings(workDirSettings);
}
}

@Deprecated
public JNLPLauncher(String tunnel, String vmargs) {
// TODO: Enable workDir by default in API? Otherwise classes inheriting from JNLPLauncher
// will need to enable the feature by default as well.
// https://github.com/search?q=org%3Ajenkinsci+%22extends+JNLPLauncher%22&type=Code
this(tunnel, vmargs, RemotingWorkDirSettings.getDisabledDefaults());
@DataBoundConstructor
public JNLPLauncher(@CheckForNull String tunnel, @CheckForNull String vmargs) {
this.tunnel = Util.fixEmptyAndTrim(tunnel);
this.vmargs = Util.fixEmptyAndTrim(vmargs);
}

/**
Expand Down Expand Up @@ -132,6 +131,11 @@ protected Object readResolve() {
public RemotingWorkDirSettings getWorkDirSettings() {
return workDirSettings;
}

@DataBoundSetter
public final void setWorkDirSettings(@Nonnull RemotingWorkDirSettings workDirSettings) {
this.workDirSettings = workDirSettings;
}

@Override
public boolean isLaunchSupported() {
Expand Down
9 changes: 6 additions & 3 deletions test/src/test/java/hudson/slaves/JNLPLauncherTest.java
Expand Up @@ -148,10 +148,9 @@ public void testNoWorkDirMigration() throws Exception {

@Test
@Issue("JENKINS-44112")
@SuppressWarnings("deprecation")
public void testDefaults() throws Exception {
String errorMsg = "Work directory should be disabled for agents created via old API";
assertTrue(errorMsg, new JNLPLauncher().getWorkDirSettings().isDisabled());
assertTrue(errorMsg, new JNLPLauncher(null, null).getWorkDirSettings().isDisabled());
assertTrue("Work directory should be disabled for agents created via old API", new JNLPLauncher().getWorkDirSettings().isDisabled());
}

@Test
Expand Down Expand Up @@ -286,8 +285,12 @@ public void testConfigRoundtrip() throws Exception {
DumbSlave s = j.createSlave();
JNLPLauncher original = new JNLPLauncher("a", "b");
s.setLauncher(original);
j.assertEqualDataBoundBeans(((JNLPLauncher) s.getLauncher()).getWorkDirSettings(), RemotingWorkDirSettings.getEnabledDefaults());
RemotingWorkDirSettings custom = new RemotingWorkDirSettings(false, null, "custom", false);
((JNLPLauncher) s.getLauncher()).setWorkDirSettings(custom);
HtmlPage p = j.createWebClient().getPage(s, "configure");
j.submit(p.getFormByName("config"));
j.assertEqualBeans(original,s.getLauncher(),"tunnel,vmargs");
j.assertEqualDataBoundBeans(((JNLPLauncher) s.getLauncher()).getWorkDirSettings(), custom);
}
}

0 comments on commit 98b3769

Please sign in to comment.