Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
49 commits
Select commit Hold shift + click to select a range
e53c48e
[JENKINS-49635] Added DirectArtifactManagerFactory test utility.
jglick Feb 28, 2018
2dd13bc
Working on ArtifactManagerTest.
jglick Mar 2, 2018
f06ee84
Rudimentary tests of VirtualFile.list overloads.
jglick Mar 12, 2018
98fef1a
Support stashes with remote artifact managers.
jglick Mar 13, 2018
5b86b27
Updates.
jglick Mar 14, 2018
4be635c
@oleg-nenashev suggests a fuller envFor implementation.
jglick Mar 14, 2018
2e6480b
Emphasizing that launcherFor and envFor are only called from deprecat…
jglick Mar 14, 2018
02f4e4d
Also testing toExternalURL.
jglick Mar 14, 2018
25bf80d
Strengthening test to cover stash & artifact deletion.
jglick Mar 15, 2018
afe16e3
Working on tests.
jglick Mar 15, 2018
ffcf02c
More tests.
jglick Mar 15, 2018
979b266
More tests.
jglick Mar 15, 2018
8fb822a
Stronger tests.
jglick Mar 16, 2018
3b63b81
More streamlined assertion of file contents.
jglick Mar 16, 2018
1da7d9a
Allow callers to decide whether to test weird characters or not.
jglick Mar 16, 2018
bcef72c
assertNonexistent
jglick Mar 16, 2018
ee7386f
Also testing VirtualFile.run(Callable) overrides.
jglick Mar 23, 2018
cb9e41e
useBeta
jglick Mar 26, 2018
70472c4
Mark StashAwareArtifactManager as beta for now, too.
jglick Mar 26, 2018
e29a3de
Testing toExternalURL rather than asRemotable.
jglick Apr 2, 2018
9003240
Picking up https://github.com/jenkinsci/plugin-pom/pull/98.
jglick Apr 3, 2018
915f652
Run artifact upload and download operations using a Dockerized agent …
jglick Apr 4, 2018
bb4098a
Porting https://github.com/jglick/workflow-api-plugin/pull/1 by @carl…
jglick Apr 4, 2018
bfe1842
Nonsensical to build against 2.89 when the baseline is a trunk snapshot.
jglick Apr 4, 2018
17887af
As with DockerClassRule, it seems necessary to be calling Docker.buil…
jglick Apr 4, 2018
dfea773
docker-fixtures 1.7
jglick Apr 5, 2018
54f2a2a
Merge branch 'master' into VirtualFile-JENKINS-49635
jglick Apr 16, 2018
395471c
Updated.
jglick Apr 16, 2018
f5cebc7
Merge branch 'master' into VirtualFile-JENKINS-49635
jglick Apr 23, 2018
44bc998
jenkins.version=2.118
jglick Apr 23, 2018
0675e9e
Preparing for Incrementals.
jglick Apr 27, 2018
77c72ff
Picking up https://github.com/jenkinsci/plugin-pom/pull/105.
jglick May 9, 2018
8e8a4c1
[JENKINS-51187] Moving git-changelist-maven-extension into incrementa…
jglick May 10, 2018
67900f6
Updates.
jglick May 11, 2018
8abe7c5
Incrementals tweaks, and may as well use an actual LTS baseline.
jglick May 16, 2018
1dfffb2
Split tests so implementations can choose
carlossg May 25, 2018
0cc8c11
Use lambdas
carlossg May 28, 2018
aa3ab38
Improve javadoc for test methods
carlossg May 28, 2018
90cc5dc
The standard manager does implement delete, so meant to call artifact…
jglick May 29, 2018
d37cbf7
Improved Javadoc for ArtifactManagerTest.
jglick Jun 1, 2018
e9b9e06
Improving DirectArtifactManagerFactory to serve actual HTTP URLs.
jglick Jun 5, 2018
175399c
Javadoc improvements.
jglick Jun 8, 2018
afd2797
Clarifying the expected usage of StashAwareArtifactManager.
jglick Jun 8, 2018
ebc0383
Merge branch 'incrementals' into VirtualFile-JENKINS-49635
jglick Jun 11, 2018
e54f030
More info on JCloudsArtifactManager.
jglick Jun 13, 2018
72d360f
Merge branch 'master' into VirtualFile-JENKINS-49635
jglick Jun 15, 2018
ea15b85
More strongly worded message.
jglick Jun 15, 2018
5b88a0d
Javadocs clarification
svanoort Jun 15, 2018
11b84be
Merge pull request #3 from svanoort/remote-docs
jglick Jun 15, 2018
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
2 changes: 1 addition & 1 deletion Jenkinsfile
Original file line number Diff line number Diff line change
@@ -1 +1 @@
buildPlugin(jenkinsVersions: [null, '2.89'])
buildPlugin()
27 changes: 26 additions & 1 deletion pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,11 @@
<properties>
<revision>2.28</revision>
<changelist>-SNAPSHOT</changelist>
<jenkins.version>2.60.3</jenkins.version>
<jenkins.version>2.121</jenkins.version>
<java.level>8</java.level>
<no-test-jar>false</no-test-jar>
<workflow-support-plugin.version>2.14</workflow-support-plugin.version>
<useBeta>true</useBeta>
</properties>
<dependencies>
<dependency>
Expand Down Expand Up @@ -140,5 +141,29 @@
<artifactId>structs</artifactId>
<version>1.14</version>
</dependency>
<dependency>
<groupId>org.jenkins-ci.test</groupId>
<artifactId>docker-fixtures</artifactId>
<version>1.8</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>ssh-slaves</artifactId>
<version>1.26</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>jdk-tool</artifactId>
<version>1.0</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>apache-httpcomponents-client-4-api</artifactId>
<version>4.5.5-2.1</version>
<scope>test</scope>
</dependency>
</dependencies>
</project>
163 changes: 149 additions & 14 deletions src/main/java/org/jenkinsci/plugins/workflow/flow/StashManager.java
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,16 @@

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import hudson.AbortException;
import hudson.EnvVars;
import hudson.Extension;
import hudson.ExtensionList;
import hudson.ExtensionPoint;
import hudson.FilePath;
import hudson.Launcher;
import hudson.Launcher.LocalLauncher;
import hudson.Util;
import hudson.model.Computer;
import hudson.model.Node;
import hudson.model.Run;
import hudson.model.TaskListener;
import hudson.org.apache.tools.tar.TarInputStream;
Expand All @@ -57,6 +61,7 @@
import org.apache.commons.io.IOUtils;
import org.apache.tools.tar.TarEntry;
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.Beta;
import org.kohsuke.accmod.restrictions.DoNotUse;
import org.kohsuke.accmod.restrictions.NoExternalUse;

Expand Down Expand Up @@ -84,20 +89,35 @@ public static void stash(@Nonnull Run<?,?> build, @Nonnull String name, @Nonnull
stash(build, name, workspace, listener, includes, excludes, useDefaultExcludes, false);
}

@Deprecated
public static void stash(@Nonnull Run<?,?> build, @Nonnull String name, @Nonnull FilePath workspace, @Nonnull TaskListener listener,
@CheckForNull String includes, @CheckForNull String excludes, boolean useDefaultExcludes, boolean allowEmpty) throws IOException, InterruptedException {
stash(build, name, workspace, launcherFor(workspace, listener), envFor(build, workspace, listener), listener, includes, excludes, useDefaultExcludes, allowEmpty);
}

/**
* Saves a stash of some files from a build.
* @param build a build to use as storage
* @param name a simple name to assign to the stash (must follow {@link Jenkins#checkGoodName} constraints)
* @param workspace a directory to use as a base
* @param launcher a way to launch processes, if required
* @param env environment to use when launching processes, if required
* @param listener a way to report progress or problems
* @param includes a set of Ant-style file includes, separated by commas; null/blank is allowed as a synonym for {@code **} (i.e., everything)
* @param excludes an optional set of Ant-style file excludes
* @param useDefaultExcludes whether to use Ant default excludes
* @param allowEmpty whether to allow an empty stash
* @see StashAwareArtifactManager#stash
*/
@SuppressFBWarnings(value="RV_RETURN_VALUE_IGNORED_BAD_PRACTICE", justification="fine if mkdirs returns false")
public static void stash(@Nonnull Run<?,?> build, @Nonnull String name, @Nonnull FilePath workspace, @Nonnull TaskListener listener,
public static void stash(@Nonnull Run<?,?> build, @Nonnull String name, @Nonnull FilePath workspace, @Nonnull Launcher launcher, @Nonnull EnvVars env, @Nonnull TaskListener listener,
Copy link
Member Author

Choose a reason for hiding this comment

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

@abayer FYI, this is the new overload which could plausibly be combined with a boolean verbose option to address JENKINS-40912 in lieu of #62. A minor API change like that could be done in trunk at low risk, leaving the introduction of StashAwareArtifactManager and core baseline changes to this PR for later.

Copy link
Member

Choose a reason for hiding this comment

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

Not entirely clear on what you mean? I made a comment over at https://issues.jenkins-ci.org/browse/JENKINS-40912 wondering whether building support for listing stash files into StashAwareArtifactManager makes sense, since anything we do here like #62 would break horribly the moment the StashAwareArtifactManager change came in... I dunno. Everything about JENKINS-40912 makes me want to light myself on fire at this point. =)

Copy link
Member Author

Choose a reason for hiding this comment

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

Responded in JIRA.

Copy link
Member

Choose a reason for hiding this comment

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

Our set of parameters is starting to get unruly - should we consider simplifying to accept some sort of StashOptions object which encapsulates some of this complexity and makes it easier to add in new options without parameter-list bloat?

Copy link
Member Author

Choose a reason for hiding this comment

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

Possible, though six are mandatory and only five optional, and there is only one caller so we are not really simplifying anything that way. Option builders are useful when you have an open-ended list of callers and most of them do not need to specify most of the options.

Copy link
Member

Choose a reason for hiding this comment

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

I'll let it go for now -- you're right that the set of callers is currently pretty limited.

@CheckForNull String includes, @CheckForNull String excludes, boolean useDefaultExcludes, boolean allowEmpty) throws IOException, InterruptedException {
Jenkins.checkGoodName(name);
StashAwareArtifactManager saam = stashAwareArtifactManager(build);
if (saam != null) {
saam.stash(name, workspace, launcher, env, listener, includes, excludes, useDefaultExcludes, allowEmpty);
return;
}
File storage = storage(build, name);
storage.getParentFile().mkdirs();
if (storage.isFile()) {
Expand All @@ -115,49 +135,88 @@ public static void stash(@Nonnull Run<?,?> build, @Nonnull String name, @Nonnull
}
}

@Deprecated
public static void unstash(@Nonnull Run<?,?> build, @Nonnull String name, @Nonnull FilePath workspace, @Nonnull TaskListener listener) throws IOException, InterruptedException {
unstash(build, name, workspace, launcherFor(workspace, listener), envFor(build, workspace, listener), listener);
}

/**
* Restores a stash of some files from a build.
* @param build a build used as storage
* @param name a name passed previously to {@link #stash}
* @param workspace a directory to copy into
* @param launcher a way to launch processes, if required
* @param env environment to use when launching processes, if required
* @param listener a way to report progress or problems
* @throws AbortException in case there is no such saved stash
* @see StashAwareArtifactManager#unstash
*/
public static void unstash(@Nonnull Run<?,?> build, @Nonnull String name, @Nonnull FilePath workspace, @Nonnull TaskListener listener) throws IOException, InterruptedException {
public static void unstash(@Nonnull Run<?,?> build, @Nonnull String name, @Nonnull FilePath workspace, @Nonnull Launcher launcher, @Nonnull EnvVars env, @Nonnull TaskListener listener) throws IOException, InterruptedException {
Jenkins.checkGoodName(name);
StashAwareArtifactManager saam = stashAwareArtifactManager(build);
if (saam != null) {
saam.unstash(name, workspace, launcher, env, listener);
return;
}
File storage = storage(build, name);
if (!storage.isFile()) {
throw new AbortException("No such saved stash ‘" + name + "’");
}
new FilePath(storage).untar(workspace, FilePath.TarCompression.GZIP);
// currently nothing to print; listener is a placeholder
}

@Deprecated
public static void clearAll(@Nonnull Run<?,?> build) throws IOException {
try {
clearAll(build, TaskListener.NULL);
} catch (InterruptedException x) {
throw new IOException(x);
}
}

/**
* Delete any and all stashes in a build.
* @param build a build possibly passed to {@link #stash} in the past
* @param listener a way to report progress or problems
* @see StashAwareArtifactManager#clearAllStashes
*/
public static void clearAll(@Nonnull Run<?,?> build) throws IOException {
public static void clearAll(@Nonnull Run<?,?> build, @Nonnull TaskListener listener) throws IOException, InterruptedException {
StashAwareArtifactManager saam = stashAwareArtifactManager(build);
if (saam != null) {
saam.clearAllStashes(listener);
return;
}
Util.deleteRecursive(storage(build));
}

@Deprecated
public static void maybeClearAll(@Nonnull Run<?,?> build) throws IOException {
try {
maybeClearAll(build, TaskListener.NULL);
} catch (InterruptedException x) {
throw new IOException(x);
}
}

/**
* Delete any and all stashes in a build unless told otherwise.
* {@link StashBehavior#shouldClearAll} may cancel this.
* @param build a build possibly passed to {@link #stash} in the past
* @see #clearAll(Run, TaskListener)
*/
public static void maybeClearAll(@Nonnull Run<?,?> build) throws IOException {
public static void maybeClearAll(@Nonnull Run<?,?> build, @Nonnull TaskListener listener) throws IOException, InterruptedException {
for (StashBehavior behavior : ExtensionList.lookup(StashBehavior.class)) {
if (!behavior.shouldClearAll(build)) {
return;
}
}
clearAll(build);
clearAll(build, listener);
}

/**
* Copy any stashes from one build to another.
* @param from a build possibly passed to {@link #stash} in the past
* @param to a new build
* @deprecated without replacement; only used from {@link CopyStashesAndArtifacts} anyway
*/
@Deprecated
public static void copyAll(@Nonnull Run<?,?> from, @Nonnull Run<?,?> to) throws IOException {
File fromStorage = storage(from);
if (!fromStorage.isDirectory()) {
Expand All @@ -166,7 +225,7 @@ public static void copyAll(@Nonnull Run<?,?> from, @Nonnull Run<?,?> to) throws
FileUtils.copyDirectory(fromStorage, storage(to));
}

@Restricted(DoNotUse.class) // currently just for tests
@Restricted(DoNotUse.class) // just for tests, and incompatible with StashAwareArtifactManager
@SuppressFBWarnings(value="DM_DEFAULT_ENCODING", justification="test code")
public static Map<String,Map<String,String>> stashesOf(@Nonnull Run<?,?> build) throws IOException {
Map<String,Map<String,String>> result = new TreeMap<String,Map<String,String>>();
Expand Down Expand Up @@ -196,11 +255,12 @@ public static Map<String,Map<String,String>> stashesOf(@Nonnull Run<?,?> build)
return result;
}

private static @Nonnull File storage(@Nonnull Run<?,?> build) {
private static @Nonnull File storage(@Nonnull Run<?,?> build) throws IOException {
assert stashAwareArtifactManager(build) == null;
return new File(build.getRootDir(), "stashes");
}

private static @Nonnull File storage(@Nonnull Run<?,?> build, @Nonnull String name) {
private static @Nonnull File storage(@Nonnull Run<?,?> build, @Nonnull String name) throws IOException {
File dir = storage(build);
File f = new File(dir, name + SUFFIX);
if (!f.getParentFile().equals(dir)) {
Expand Down Expand Up @@ -229,16 +289,91 @@ public boolean shouldClearAll(@Nonnull Run<?,?> build) {

}

/**
* Mixin interface for an {@link ArtifactManager} which supports specialized stash behavior as well.
*
* <p> When implementing off-Jenkins artifact storage, you should NOT extend this directly but instead use the
* {@code JCloudsArtifactManager} in the plugin currently named {@code artifact-manager-s3}.
*
* This is dangerous to directly extend if using remote storage unless you write a very robust handling of network failures including at least a base timeout and retries.
* The {@code JCloudsArtifactManager} implementation supports extensibility to various cloud providers and custom stores via the {@code BlobStoreProvider} ExtensionPoint.
* It handles all aspects of making cloud artifact storage work smoothly in Jenkins
* including the {@link VirtualFile} implementation, robust network error handling, overall configuration UI, and more.
* <strong>Implement this interface directly at your own risk.</strong>
* @see <a href="https://github.com/jenkinsci/jep/blob/master/jep/202/README.adoc">JEP-202</a>
*/
@Restricted(Beta.class)
public interface StashAwareArtifactManager /* extends ArtifactManager */ {

/** @see StashManager#stash(Run, String, FilePath, Launcher, EnvVars, TaskListener, String, String, boolean, boolean) */
void stash(@Nonnull String name, @Nonnull FilePath workspace, @Nonnull Launcher launcher, @Nonnull EnvVars env, @Nonnull TaskListener listener, @CheckForNull String includes, @CheckForNull String excludes, boolean useDefaultExcludes, boolean allowEmpty) throws IOException, InterruptedException;
Copy link
Member

Choose a reason for hiding this comment

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

Again, we should really be looking at an options object to reduce the parameter complexity.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ditto, only one caller.


/** @see StashManager#unstash(Run, String, FilePath, Launcher, EnvVars, TaskListener) */
void unstash(@Nonnull String name, @Nonnull FilePath workspace, @Nonnull Launcher launcher, @Nonnull EnvVars env, @Nonnull TaskListener listener) throws IOException, InterruptedException;

/** @see StashManager#clearAll(Run, TaskListener) */
void clearAllStashes(@Nonnull TaskListener listener) throws IOException, InterruptedException;

/**
* Copy all stashes and artifacts from one build to another.
* The {@link ArtifactManager} configuration will be as of the origin build.
* If the implementation cannot handle {@code to} for whatever reason, it may throw {@link AbortException}.
* @see CopyStashesAndArtifacts
*/
void copyAllArtifactsAndStashes(@Nonnull Run<?,?> to, @Nonnull TaskListener listener) throws IOException, InterruptedException;

}

private static @CheckForNull StashAwareArtifactManager stashAwareArtifactManager(@Nonnull Run<?, ?> build) throws IOException {
ArtifactManager am = build.pickArtifactManager();
return am instanceof StashAwareArtifactManager ? (StashAwareArtifactManager) am : null;
}

@Deprecated
private static @Nonnull Launcher launcherFor(@Nonnull FilePath workspace, @Nonnull TaskListener listener) {
Copy link
Member

Choose a reason for hiding this comment

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

Question: are there extension points around Launcher creation that we might want to support interaction with?

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, handled by Node.createLauncher. It does not matter since this is dead code unless the user did not also update workflow-basic-steps.

Copy link
Member

Choose a reason for hiding this comment

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

👍

Computer c = workspace.toComputer();
if (c != null) {
Node n = c.getNode();
if (n != null) {
return n.createLauncher(listener);
} else {
listener.error(c.getDisplayName() + " seems to be offline");
return new LocalLauncher(listener);
}
} else {
listener.error(workspace + " seems to be offline");
return new LocalLauncher(listener);
}
}

@Deprecated
private static @Nonnull EnvVars envFor(@Nonnull Run<?, ?> build, @Nonnull FilePath workspace, @Nonnull TaskListener listener) throws IOException, InterruptedException {
Copy link
Member

Choose a reason for hiding this comment

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

Wait... why did we introduce a new deprecated method?

Copy link
Member Author

Choose a reason for hiding this comment

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

See above, this is dead code for binary compatibility.

Computer c = workspace.toComputer();
if (c != null) {
EnvVars e = c.getEnvironment();
e.putAll(c.buildEnvironment(listener));
e.putAll(build.getEnvironment(listener));
return e;
} else {
listener.error(workspace + " seems to be offline");
return new EnvVars();
}
}

@Restricted(NoExternalUse.class)
@Extension public static class CopyStashesAndArtifacts extends FlowCopier.ByRun {

@Override public void copy(Run<?,?> original, Run<?,?> copy, TaskListener listener) throws IOException, InterruptedException {
// TODO ArtifactManager should define an optimized operation to copy from another, or VirtualFile should define copyRecursive
StashAwareArtifactManager saam = stashAwareArtifactManager(original);
if (saam != null) {
saam.copyAllArtifactsAndStashes(copy, listener);
return;
}
VirtualFile srcroot = original.getArtifactManager().root();
FilePath dstDir = createTmpDir();
try {
Map<String,String> files = new HashMap<>();
for (String path : srcroot.list("**/*")) {
for (String path : srcroot.list("**/*", null, false)) {
files.put(path, path);
InputStream in = srcroot.child(path).open();
try {
Expand Down
Loading