Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Nitpicking #1

Merged
merged 3 commits into from

2 participants

@kohsuke
Owner

Mark found out a few glitches in the plugin. These two changes eliminate them.

The first commit is a bit more serious in that it will interfere with the correctness of the plugin when it's used in a certain project type, such as a Maven project. It also prevents the job configuration from getting updated programmatically (although this isn't probably too common.)

The second commit removes a rough edge where a form validation is too eager and issue a big warning where it shouldn't.

kohsuke and others added some commits
@kohsuke kohsuke FilePath is not persistable as a configuration.
... because it contains references to the transient objects like a channel connection to a slave. So at minimum the field needs to be marked 'transient'.

Builder can be also shared across multiple builds, so the best thing to do in this case is not to store this state in the field. There's an up-to-date check inside the performInstallation method so that this lack of caching shouldn't really result in a notcable performance degradation.
0a458f6
@kohsuke kohsuke Relaxing the form validation a little bit.
If the variable references are used inside the setting, they will cause the validation to fail and issue a false positive warning, and that'd be confusing.
I'm fixing this in FilePath.validateFileMask, but in the mean time a workaround is implemented in a plugin.
61d7f2a
@msolnit msolnit Remove some comments that no longer apply. c9d5dcb
@msolnit msolnit was assigned
@msolnit
Collaborator

Thanks Kohsuke. I'm merging, but can you elaborate on this part?

The first commit is a bit more serious in that it will interfere
with the correctness of the plugin when it's used in a certain
project type, such as a Maven project.

@msolnit msolnit merged commit 1d2546b into master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 3, 2013
  1. @kohsuke

    FilePath is not persistable as a configuration.

    kohsuke authored
    ... because it contains references to the transient objects like a channel connection to a slave. So at minimum the field needs to be marked 'transient'.
    
    Builder can be also shared across multiple builds, so the best thing to do in this case is not to store this state in the field. There's an up-to-date check inside the performInstallation method so that this lack of caching shouldn't really result in a notcable performance degradation.
  2. @kohsuke

    Relaxing the form validation a little bit.

    kohsuke authored
    If the variable references are used inside the setting, they will cause the validation to fail and issue a false positive warning, and that'd be confusing.
    I'm fixing this in FilePath.validateFileMask, but in the mean time a workaround is implemented in a plugin.
Commits on Apr 4, 2013
  1. @msolnit
This page is out of date. Refresh to see the latest.
View
16 src/main/java/com/soasta/jenkins/AbstractCloudTestBuilderDescriptor.java
@@ -4,12 +4,15 @@
*/
package com.soasta.jenkins;
+import hudson.FilePath;
import hudson.model.AbstractProject;
import hudson.tasks.BuildStepDescriptor;
import hudson.tasks.Builder;
+import hudson.util.FormValidation;
import hudson.util.ListBoxModel;
import javax.inject.Inject;
+import java.io.IOException;
/**
* @author Kohsuke Kawaguchi
@@ -34,4 +37,17 @@ public ListBoxModel doFillUrlItems() {
public boolean showUrlField() {
return serverDescriptor.getServers().size()>1;
}
+
+ protected FormValidation validateFileMask(AbstractProject project, String value) throws IOException {
+ if (value.contains("${")) {
+ // if the value contains a variable reference, bail out from the check because we can end up
+ // warning a file that actually resolves correctly at the runtime
+ // the same change is made in FilePath.validateFileMask independently, and in the future
+ // we can remove this check from here
+ return FormValidation.ok();
+ }
+
+ // Make sure the file exists.
+ return FilePath.validateFileMask(project.getSomeWorkspace(), value);
+ }
}
View
11 src/main/java/com/soasta/jenkins/AbstractSCommandBuilder.java
@@ -23,8 +23,6 @@
*/
private final String url;
- private FilePath scommand;
-
public AbstractSCommandBuilder(String url) {
this.url = url;
}
@@ -43,14 +41,7 @@ protected ArgumentListBuilder getSCommandArgs(AbstractBuild<?, ?> build, BuildLi
throw new AbortException("No TouchTest server is configured in the system configuration.");
// Download SCommand, if needed.
-
- // We remember the location for next time, since this might be called
- // more than once for a single build step (e.g. TestCompositionRunner
- // with a list of compositions).
-
- // As far as I know, this null check does not need to be thread-safe.
- if (scommand == null)
- scommand = new SCommandInstaller(s).scommand(build.getBuiltOn(), listener);
+ FilePath scommand = new SCommandInstaller(s).scommand(build.getBuiltOn(), listener);
ArgumentListBuilder args = new ArgumentListBuilder();
args.add(scommand)
View
2  src/main/java/com/soasta/jenkins/ImportFiles.java
@@ -123,7 +123,7 @@ public String getDisplayName() {
*/
public FormValidation doCheckFiles(@AncestorInPath AbstractProject project, @QueryParameter String value) throws IOException {
String includes = convertFileListToIncludePattern(value);
- return FilePath.validateFileMask(project.getSomeWorkspace(), includes);
+ return validateFileMask(project, includes);
}
public ListBoxModel doFillModeItems() {
View
2  src/main/java/com/soasta/jenkins/MakeAppTouchTestable.java
@@ -129,7 +129,7 @@ public FormValidation doCheckProjectFile(@AncestorInPath AbstractProject project
return FormValidation.error("Project directory is required.");
} else {
// Make sure the directory exists.
- return FilePath.validateFileMask(project.getSomeWorkspace(), value);
+ return validateFileMask(project, value);
}
}
}
View
2  src/main/java/com/soasta/jenkins/iOSAppInstaller.java
@@ -55,7 +55,7 @@ public FormValidation doCheckIpa(@AncestorInPath AbstractProject project, @Query
return FormValidation.error("IPA file is required.");
} else {
// Make sure the file exists.
- return FilePath.validateFileMask(project.getSomeWorkspace(), value);
+ return validateFileMask(project, value);
}
}
}
View
2  src/main/java/com/soasta/jenkins/iOSSimulatorLauncher.java
@@ -109,7 +109,7 @@ public FormValidation doCheckApp(@AncestorInPath AbstractProject project, @Query
return FormValidation.error("App directory is required.");
} else {
// Make sure the directory exists.
- return FilePath.validateFileMask(project.getSomeWorkspace(), value);
+ return validateFileMask(project, value);
}
}
}
Something went wrong with that request. Please try again.