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.

Sign up for GitHub

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-34122] Exclude output file from itself #19

Closed
wants to merge 5 commits into from
Closed

[JENKINS-34122] Exclude output file from itself #19

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1294911
Exclude output file from itself
alecharp Oct 4, 2016
1e61f00
Validate implementation with unit-test
alecharp Oct 4, 2016
fda998a
make the tests pass on windows
jtnord Oct 12, 2016
46717d2
Unit test to make sure we can include a file with the same name
jtnord Oct 13, 2016
a6ee28a
Alternative implementation
jtnord Oct 13, 2016
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
13 changes: 12 additions & 1 deletion src/main/java/org/jenkinsci/plugins/pipeline/utility/steps/zip/ZipStepExecution.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,12 +120,23 @@ public ZipItFileCallable(FilePath zipFile, String glob) {

@Override
public Integer invoke(File dir, VirtualChannel channel) throws IOException, InterruptedException {
String canonicalZip = new File(zipFile.getRemote()).getCanonicalPath();
System.out.println("zipFileRemote is: " + zipFile.getRemote());
System.out.println("canonicalZip is: " + canonicalZip);
Copy link
Member

Choose a reason for hiding this comment

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

Is this normal to sysout instead of logging?


Archiver archiver = ArchiverFactory.ZIP.create(zipFile.write());
FileSet fs = Util.createFileSet(dir, glob);
DirectoryScanner scanner = fs.getDirectoryScanner(new org.apache.tools.ant.Project());
try {
for (String path : scanner.getIncludedFiles()) {
archiver.visit(new File(dir, path), path);
File toArchive = new File(dir, path).getCanonicalFile();
if (toArchive.getPath().equals(canonicalZip)) {
System.out.println("Not archiving " + toArchive + " as this is the output zip itself");
Copy link
Member

Choose a reason for hiding this comment

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

ditto

}
else {
System.out.println("archiving " + toArchive + " as this is not excluded");
Copy link
Member

Choose a reason for hiding this comment

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

same

archiver.visit(toArchive, path);
}
}
} finally {
archiver.close();
Expand Down
10 changes: 5 additions & 5 deletions src/test/java/org/jenkinsci/plugins/pipeline/utility/steps/conf/ReadPropertiesStepTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ public void readFile() throws Exception {
WorkflowJob p = j.jenkins.createProject(WorkflowJob.class, "p");
p.setDefinition(new CpsFlowDefinition(
"node('slaves') {\n" +
" def props = readProperties file: '" + file.getAbsolutePath() + "'\n" +
" def props = readProperties file: '" + file.getAbsolutePath().replace('\\', '/') + "'\n" +
" assert props['test'] == 'One'\n" +
" assert props['another'] == 'Two'\n" +
" assert props.test == 'One'\n" +
Expand All @@ -91,7 +91,7 @@ public void readFileWithDefaults() throws Exception {
p.setDefinition(new CpsFlowDefinition(
"node('slaves') {\n" +
" def d = [test: 'Default', something: 'Default']\n" +
" def props = readProperties defaults: d, file: '" + file.getAbsolutePath() + "'\n" +
" def props = readProperties defaults: d, file: '" + file.getAbsolutePath().replace('\\', '/') + "'\n" +
" assert props['test'] == 'One'\n" +
" assert props['another'] == 'Two'\n" +
" assert props.test == 'One'\n" +
Expand All @@ -115,7 +115,7 @@ public void readText() throws Exception {
WorkflowJob p = j.jenkins.createProject(WorkflowJob.class, "p");
p.setDefinition(new CpsFlowDefinition(
"node('slaves') {\n" +
" String propsText = readFile file: '" + file.getAbsolutePath() + "'\n" +
" String propsText = readFile file: '" + file.getAbsolutePath().replace('\\', '/') + "'\n" +
" def props = readProperties text: propsText\n" +
" assert props['test'] == 'One'\n" +
" assert props['another'] == 'Two'\n" +
Expand Down Expand Up @@ -170,8 +170,8 @@ public void readFileAndText() throws Exception {
WorkflowJob p = j.jenkins.createProject(WorkflowJob.class, "p");
p.setDefinition(new CpsFlowDefinition(
"node('slaves') {\n" +
" String propsText = readFile file: '" + textFile.getAbsolutePath() + "'\n" +
" def props = readProperties text: propsText, file: '" + file.getAbsolutePath() + "'\n" +
" String propsText = readFile file: '" + textFile.getAbsolutePath().replace('\\', '/') + "'\n" +
" def props = readProperties text: propsText, file: '" + file.getAbsolutePath().replace('\\', '/') + "'\n" +
" assert props['test'] == 'One'\n" +
" assert props.test == 'One'\n" +
" assert props['text'] == 'TextOne'\n" +
Expand Down
10 changes: 6 additions & 4 deletions src/test/java/org/jenkinsci/plugins/pipeline/utility/steps/conf/mf/ReadManifestStepTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ public void configRoundTrip() throws Exception {
public void testJarWithGradleManifest() throws Exception {
URL resource = getClass().getResource("gradle-manifest.war");

String remoting = new File(resource.getPath()).getAbsolutePath();
String remoting = new File(resource.getPath()).getAbsolutePath().replace('\\', '/');
p.setDefinition(new CpsFlowDefinition(
"node('slaves') {\n" +
" def man = readManifest file: '" + remoting + "'\n" +
Expand All @@ -86,7 +86,7 @@ public void testJarWithGradleManifest() throws Exception {

@Test
public void testJar() throws Exception {
String remoting = new File(j.getWebAppRoot(), "WEB-INF/remoting.jar").getAbsolutePath();
String remoting = new File(j.getWebAppRoot(), "WEB-INF/remoting.jar").getAbsolutePath().replace('\\', '/');
p.setDefinition(new CpsFlowDefinition(
"node('slaves') {\n" +
" def man = readManifest file: '" + remoting + "'\n" +
Expand All @@ -106,9 +106,10 @@ public void testJar() throws Exception {
@Test
public void testText() throws Exception {
URL resource = getClass().getResource("testmanifest.mf");
File f = new File(resource.toURI());
p.setDefinition(new CpsFlowDefinition(
"node('slaves') {\n" +
" String txt = readFile '"+resource.getPath()+"'\n" +
" String txt = readFile '"+f.getPath().replace('\\', '/')+"'\n" +
" def man = readManifest text: txt\n" +
" assert man != null\n" +
" assert man.main != null\n" +
Expand All @@ -126,9 +127,10 @@ public void testText() throws Exception {
@Test
public void testFile() throws Exception {
URL resource = getClass().getResource("testmanifest.mf");
File f = new File(resource.toURI());
p.setDefinition(new CpsFlowDefinition(
"node('slaves') {\n" +
" def man = readManifest file: '"+resource.getPath()+"'\n" +
" def man = readManifest file: '"+f.getPath().replace('\\', '/')+"'\n" +
" assert man != null\n" +
" assert man.main != null\n" +
" echo man.main['Version']\n" +
Expand Down
6 changes: 3 additions & 3 deletions src/test/java/org/jenkinsci/plugins/pipeline/utility/steps/fs/FindFilesStepTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ public void simpleList() throws Exception {
"def files = findFiles()\n" +
"echo \"${files.length} files\"\n" +
"for(int i = 0; i < files.length; i++) {\n" +
" echo \"F: ${files[i]}\"\n" +
" echo \"F: ${files[i].path.replace('\\\\', '/')}\"\n" +
"}"
);
p.setDefinition(new CpsFlowDefinition(flow, false));
Expand All @@ -101,7 +101,7 @@ public void listAll() throws Exception {
"def files = findFiles(glob: '**/*.txt')\n" +
"echo \"${files.length} files\"\n" +
"for(int i = 0; i < files.length; i++) {\n" +
" echo \"F: ${files[i]}\"\n" +
" echo \"F: ${files[i].path.replace('\\\\', '/')}\"\n" +
"}"
);
p.setDefinition(new CpsFlowDefinition(flow, false));
Expand All @@ -128,7 +128,7 @@ public void listSome() throws Exception {
"def files = findFiles(glob: '**/a/*.txt')\n" +
"echo \"${files.length} files\"\n" +
"for(int i = 0; i < files.length; i++) {\n" +
" echo \"F: ${files[i]}\"\n" +
" echo \"F: ${files[i].path.replace('\\\\', '/')}\"\n" +
"}"
);
p.setDefinition(new CpsFlowDefinition(flow, false));
Expand Down
68 changes: 64 additions & 4 deletions src/test/java/org/jenkinsci/plugins/pipeline/utility/steps/zip/ZipStepTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,7 @@
import java.util.zip.ZipEntry;
import java.util.zip.ZipInputStream;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.*;

/**
* Tests for {@link ZipStep}.
Expand Down Expand Up @@ -91,6 +88,59 @@ public void simpleArchivedZip() throws Exception {

}

@Test
public void shouldNotPutOutputArchiveIntoItself() throws Exception {

WorkflowJob p = j.jenkins.createProject(WorkflowJob.class, "p");
p.setDefinition(new CpsFlowDefinition(
"node {" +
" writeFile file: 'hello.txt', text: 'Hello world'\n" +
" zip zipFile: 'output.zip', dir: '', glob: '', archive: true\n" +
"}", false));
WorkflowRun run = j.assertBuildStatusSuccess(p.scheduleBuild2(0));
run = j.assertBuildStatusSuccess(p.scheduleBuild2(0));
j.assertLogContains("Writing zip file", run);
verifyArchivedNotContainingItself(run);
}

@Test
public void canArchiveFileWithSameName() throws Exception {
WorkflowJob p = j.jenkins.createProject(WorkflowJob.class, "p");
p.setDefinition(new CpsFlowDefinition(
"node {\n" +
" dir ('src') {\n" +
" writeFile file: 'hello.txt', text: 'Hello world'\n" +
" writeFile file: 'output.zip', text: 'not really a zip'\n" +
" }\n" +
" dir ('out') {\n" +
" zip zipFile: 'output.zip', dir: '../src', glob: '', archive: true\n" +
" }\n" +
"}\n",
false));
WorkflowRun run = j.assertBuildStatusSuccess(p.scheduleBuild2(0));
run = j.assertBuildStatusSuccess(p.scheduleBuild2(0));

j.assertLogContains("Writing zip file", run);
assertTrue("Build should have artifacts", run.getHasArtifacts());
Run<WorkflowJob, WorkflowRun>.Artifact artifact = run.getArtifacts().get(0);
assertEquals("output.zip", artifact.getFileName());
VirtualFile file = run.getArtifactManager().root().child(artifact.relativePath);
ZipInputStream zip = new ZipInputStream(file.open());
ZipEntry entry = zip.getNextEntry();
while (entry != null && !entry.getName().equals("output.zip")) {
System.out.println("zip entry name is: " + entry.getName());
Copy link
Member

Choose a reason for hiding this comment

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

Can live with it in a test, though I don't like it much.

entry = zip.getNextEntry();
}
assertNotNull("output.zip should be included in the zip", entry);
// we should have the the zip - but double check
assertEquals("output.zip", entry.getName());
Scanner scanner = new Scanner(zip);
assertTrue(scanner.hasNextLine());
// the file that was not a zip should be included.
assertEquals("not really a zip", scanner.nextLine());
zip.close();
}

@Test
public void globbedArchivedZip() throws Exception {

Expand Down Expand Up @@ -153,4 +203,14 @@ private void verifyArchivedHello(WorkflowRun run, String basePath) throws IOExce
assertNull("There should be no more entries", zip.getNextEntry());
zip.close();
}

private void verifyArchivedNotContainingItself(WorkflowRun run) throws IOException {
assertTrue("Build should have artifacts", run.getHasArtifacts());
Run<WorkflowJob, WorkflowRun>.Artifact artifact = run.getArtifacts().get(0);
VirtualFile file = run.getArtifactManager().root().child(artifact.relativePath);
ZipInputStream zip = new ZipInputStream(file.open());
for(ZipEntry entry = zip.getNextEntry(); entry != null; entry = zip.getNextEntry()) {
assertNotEquals("The zip output file shouldn't contain itself", entry.getName(), artifact.getFileName());
}
}
}