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

Jump to bottom

Minor cleanups after merging WAR Gradle support contribution #1072

Merged
merged 5 commits into from
Oct 1, 2018
Merged

Minor cleanups after merging WAR Gradle support contribution #1072

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
44f1fe9
Minor cleanups
chanseokoh Oct 1, 2018
d060bb7
Remove duplicate
chanseokoh Oct 1, 2018
4a81310
Rename variable / simplify usage/ more cleanup
chanseokoh Oct 1, 2018
0f872b8
Revert Path and use File in ExplodedWarTask
chanseokoh Oct 1, 2018
17f63cf
Fix test due to Path-to-File change
chanseokoh Oct 1, 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
4 changes: 2 additions & 2 deletions ...ore/src/test/java/com/google/cloud/tools/jib/frontend/JavaDockerContextGeneratorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ public void testMakeDockerfile() throws IOException {
}

@Test
public void testMakeDockerfileWithWebapp() throws IOException {
public void testMakeDockerfileWithWebApp() throws IOException {
String expectedBaseImage = "tomcat:8.5-jre8-alpine";
AbsoluteUnixPath exepectedAppRoot = AbsoluteUnixPath.get("/usr/local/tomcat/webapps/ROOT/");

Expand Down Expand Up @@ -197,7 +197,7 @@ public void testMakeDockerfileWithWebapp() throws IOException {
// Need to split/rejoin the string here to avoid cross-platform troubles
List<String> sampleDockerfile =
Resources.readLines(
Resources.getResource("webappSampleDockerfile"), StandardCharsets.UTF_8);
Resources.getResource("webAppSampleDockerfile"), StandardCharsets.UTF_8);
Assert.assertEquals(String.join("\n", sampleDockerfile), dockerfile);
}
}
41 changes: 16 additions & 25 deletions jib-core/src/test/java/com/google/cloud/tools/jib/frontend/JavaLayerConfigurationsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,46 +161,37 @@ public void testAddFile_regularFiles() throws IOException, URISyntaxException {

@Test
public void testWebApp() throws IOException {
AbsoluteUnixPath expectedAppRoot = AbsoluteUnixPath.get("/usr/local/tomcat/webapps/ROOT/");
JavaLayerConfigurations.Builder layerBuilder = JavaLayerConfigurations.builder();
AbsoluteUnixPath appRoot = AbsoluteUnixPath.get("/usr/local/tomcat/webapps/ROOT/");

JavaLayerConfigurations configurations =
layerBuilder
.addResourceFile(Paths.get("test.jsp"), expectedAppRoot.resolve("test.jsp"))
.addResourceFile(Paths.get("META-INF/"), expectedAppRoot.resolve("META-INF/"))
.addResourceFile(
Paths.get("context.xml"), expectedAppRoot.resolve("WEB-INF/context.xml"))
.addResourceFile(Paths.get("sub_dir/"), expectedAppRoot.resolve("WEB-INF/sub_dir/"))
.addDependencyFile(
Paths.get("myLib.jar"), expectedAppRoot.resolve("WEB-INF/lib/myLib.jar"))
JavaLayerConfigurations.builder()
.addResourceFile(Paths.get("test.jsp"), appRoot.resolve("test.jsp"))
.addResourceFile(Paths.get("META-INF/"), appRoot.resolve("META-INF/"))
.addResourceFile(Paths.get("context.xml"), appRoot.resolve("WEB-INF/context.xml"))
.addResourceFile(Paths.get("sub_dir/"), appRoot.resolve("WEB-INF/sub_dir/"))
.addDependencyFile(Paths.get("myLib.jar"), appRoot.resolve("WEB-INF/lib/myLib.jar"))
.addSnapshotDependencyFile(
Paths.get("my-SNAPSHOT.jar"),
expectedAppRoot.resolve("WEB-INF/lib/my-SNAPSHOT.jar"))
.addClassFile(
Paths.get("test.class"), expectedAppRoot.resolve("WEB-INF/classes/test.class"))
Paths.get("my-SNAPSHOT.jar"), appRoot.resolve("WEB-INF/lib/my-SNAPSHOT.jar"))
.addClassFile(Paths.get("test.class"), appRoot.resolve("WEB-INF/classes/test.class"))
.addExtraFile(Paths.get("extra.file"), AbsoluteUnixPath.get("/extra.file"))
.build();

ImmutableList<LayerEntry> expectedDependenciesLayer =
ImmutableList.of(
new LayerEntry(
Paths.get("myLib.jar"), expectedAppRoot.resolve("WEB-INF/lib/myLib.jar")));
new LayerEntry(Paths.get("myLib.jar"), appRoot.resolve("WEB-INF/lib/myLib.jar")));
ImmutableList<LayerEntry> expectedSnapshotDependenciesLayer =
ImmutableList.of(
new LayerEntry(
Paths.get("my-SNAPSHOT.jar"),
expectedAppRoot.resolve("WEB-INF/lib/my-SNAPSHOT.jar")));
Paths.get("my-SNAPSHOT.jar"), appRoot.resolve("WEB-INF/lib/my-SNAPSHOT.jar")));
ImmutableList<LayerEntry> expectedResourcesLayer =
ImmutableList.of(
new LayerEntry(Paths.get("test.jsp"), expectedAppRoot.resolve("test.jsp")),
new LayerEntry(Paths.get("META-INF"), expectedAppRoot.resolve("META-INF")),
new LayerEntry(
Paths.get("context.xml"), expectedAppRoot.resolve("WEB-INF/context.xml")),
new LayerEntry(Paths.get("sub_dir"), expectedAppRoot.resolve("WEB-INF/sub_dir")));
new LayerEntry(Paths.get("test.jsp"), appRoot.resolve("test.jsp")),
new LayerEntry(Paths.get("META-INF"), appRoot.resolve("META-INF")),
new LayerEntry(Paths.get("context.xml"), appRoot.resolve("WEB-INF/context.xml")),
new LayerEntry(Paths.get("sub_dir"), appRoot.resolve("WEB-INF/sub_dir")));
ImmutableList<LayerEntry> expectedClassesLayer =
ImmutableList.of(
new LayerEntry(
Paths.get("test.class"), expectedAppRoot.resolve("WEB-INF/classes/test.class")));
new LayerEntry(Paths.get("test.class"), appRoot.resolve("WEB-INF/classes/test.class")));
ImmutableList<LayerEntry> expectedExtraLayer =
ImmutableList.of(
new LayerEntry(Paths.get("extra.file"), AbsoluteUnixPath.get("/extra.file")));
Expand Down
0 ...src/test/resources/webappSampleDockerfile → ...src/test/resources/webAppSampleDockerfile
View file
Open in desktop
File renamed without changes.
9 changes: 5 additions & 4 deletions jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/ExplodedWarTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,18 +22,19 @@
import org.gradle.api.tasks.OutputDirectory;
import org.gradle.api.tasks.Sync;

/** This Gradle Task explodes a War file into a directory */
/** Gradle task that explodes a WAR file into a directory. */
public class ExplodedWarTask extends Sync {

@Nullable private File explodedWarDirectory;

public void setWarFile(File warFile) {
public void setWarFile(Path warFile) {
from(getProject().zipTree(warFile));
}

/**
* Sets the output directory of Sync Task
* Sets the exploded WAR output directory of this {@link Sync} task.
*
* @param explodedWarDirectory the directory where to extract the war file
* @param explodedWarDirectory the directory where to extract the WAR file
*/
public void setExplodedWarDirectory(Path explodedWarDirectory) {
this.explodedWarDirectory = explodedWarDirectory.toFile();
Expand Down
67 changes: 31 additions & 36 deletions ...dle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/GradleLayerConfigurations.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import com.google.cloud.tools.jib.frontend.JavaLayerConfigurations.Builder;
import java.io.File;
import java.io.IOException;
import java.nio.file.FileSystems;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.ArrayList;
Expand All @@ -42,23 +41,17 @@ class GradleLayerConfigurations {
/** Name of the `main` {@link SourceSet} to use as source files. */
private static final String MAIN_SOURCE_SET_NAME = "main";

/** The filename suffix for a maven/gradle snapshot dependency */
/** The filename suffix for snapshot dependency JARs. */
private static final String SNAPSHOT = "SNAPSHOT";
/** The standard directory name containing libs, classes, web.xml, etc... in a War Project */
private static final String WEB_INF = "WEB-INF";
/** The standard directory name containing libs and snapshot-libs in a War Project */
private static final String WEB_INF_LIB = WEB_INF + "/lib/";
/** The standard directory name containing classes and some resources in a War Project */
private static final String WEB_INF_CLASSES = WEB_INF + "/classes/";

/**
* Resolves the {@link JavaLayerConfigurations} for a Gradle {@link Project}.
*
* @param project the Gradle {@link Project}
* @param logger the logger for providing feedback about the resolution
* @param extraDirectory path to the directory for the extra files layer
* @param extraDirectory path to the source directory for the extra files layer
* @param appRoot root directory in the image where the app will be placed
* @return a {@link JavaLayerConfigurations} for the layers for the Gradle {@link Project}
* @return {@link JavaLayerConfigurations} for the layers for the Gradle {@link Project}
* @throws IOException if an I/O exception occurred during resolution
*/
static JavaLayerConfigurations getForProject(
Expand All @@ -77,9 +70,9 @@ static JavaLayerConfigurations getForProject(
*
* @param project the Gradle {@link Project}
* @param logger the logger for providing feedback about the resolution
* @param extraDirectory path to the directory for the extra files layer
* @param extraDirectory path to the source directory for the extra files layer
* @param appRoot root directory in the image where the app will be placed
* @return a {@link JavaLayerConfigurations} for the layers for the Gradle {@link Project}
* @return {@link JavaLayerConfigurations} for the layers for the Gradle {@link Project}
* @throws IOException if an I/O exception occurred during resolution
*/
private static JavaLayerConfigurations getForNonWarProject(
Expand Down Expand Up @@ -169,9 +162,9 @@ private static JavaLayerConfigurations getForNonWarProject(
*
* @param project the Gradle {@link Project}
* @param logger the build logger for providing feedback about the resolution
* @param extraDirectory path to the directory for the extra files layer
* @param extraDirectory path to the source directory for the extra files layer
* @param appRoot root directory in the image where the app will be placed
* @return a {@link JavaLayerConfigurations} for the layers for the Gradle {@link Project}
* @return {@link JavaLayerConfigurations} for the layers for the Gradle {@link Project}
* @throws IOException if an I/O exception occurred during resolution
*/
private static JavaLayerConfigurations getForWarProject(
Expand All @@ -185,7 +178,7 @@ private static JavaLayerConfigurations getForWarProject(

Path explodedWarPath = GradleProjectProperties.getExplodedWarDirectory(project);

Path libOutputDirectory = explodedWarPath.resolve(WEB_INF_LIB);
Path libOutputDirectory = explodedWarPath.resolve("WEB-INF/lib");
Copy link
Contributor

Choose a reason for hiding this comment

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

What was the reason for removing the constants here?

Copy link
Member Author

@chanseokoh chanseokoh Oct 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

There are pros and cons, but I thought pros of removing constants have more benefits, particularly for readability.

  • WEB_INF_LIB will always be "WEB-INF/lib". If it's something we may change in the future (like some default value of our choice), I can see a good reason for having the constant declared in the top of the file, but in this case, it's always static.
  • Readability: I keep looking into the actual value of WEB_INB_LIB to see what libOutputDirectory actually is. With "WEB-INF/lib", it's directly recognizable.
  • Looking at the field WEB_INF_LIB makes me wonder at which places this constant is being referenced to.

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you think? I'm actually fine with other ways.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay that sounds good 👍

try (Stream<Path> dependencyFileStream = Files.list(libOutputDirectory)) {
dependencyFileStream.forEach(
path -> {
Expand All @@ -196,13 +189,15 @@ private static JavaLayerConfigurations getForWarProject(
}
});
}
// All files except classes and libs in resources

// First, all files except WEB-INF go into the resources layer.
try (Stream<Path> fileStream = Files.list(explodedWarPath)) {
fileStream.filter(path -> !path.endsWith(WEB_INF)).forEach(resourcesFiles::add);
fileStream.filter(path -> !path.endsWith("WEB-INF")).forEach(resourcesFiles::add);
}
// Some files in WEB-INF need to be in resources directory (e.g. web.xml, ...)
Path webinfOutputDirectory = explodedWarPath.resolve(WEB_INF);
try (Stream<Path> fileStream = Files.list(webinfOutputDirectory)) {
// Some files in WEB-INF/ (e.g. web.xml, ...) need to go into the resources layer. However,
// don't add or look into WEB-INF/classes and WEB-INF/lib.
Path webInfOutputDirectory = explodedWarPath.resolve("WEB-INF");
try (Stream<Path> fileStream = Files.list(webInfOutputDirectory)) {
fileStream
.filter(path -> !path.endsWith("classes") && !path.endsWith("lib"))
.forEach(resourcesWebInfFiles::add);
Expand All @@ -216,31 +211,31 @@ private static JavaLayerConfigurations getForWarProject(
}

Builder layerBuilder = JavaLayerConfigurations.builder();
AbsoluteUnixPath dependenciesExtractionPath = appRoot.resolve(WEB_INF_LIB);
AbsoluteUnixPath classesExtractionPath = appRoot.resolve(WEB_INF_CLASSES);
AbsoluteUnixPath webInfExtractionPath = appRoot.resolve(WEB_INF);
AbsoluteUnixPath dependenciesExtractionPath = appRoot.resolve("WEB-INF/lib");
AbsoluteUnixPath classesExtractionPath = appRoot.resolve("WEB-INF/classes");
AbsoluteUnixPath webInfExtractionPath = appRoot.resolve("WEB-INF");

// For "WEB-INF/classes", *.class in class layer, other files and empty directories in resource
// layer
Path srcWebInfClasses = explodedWarPath.resolve(WEB_INF_CLASSES);
new DirectoryWalker(srcWebInfClasses)
// For "WEB-INF/classes", *.class go into the class layer. All other files and empty directories
// go into the resource layer.
Path webInfClasses = explodedWarPath.resolve("WEB-INF/classes");
new DirectoryWalker(webInfClasses)
.walk(
path -> {
if (FileSystems.getDefault().getPathMatcher("glob:**.class").matches(path)) {
layerBuilder.addClassFile(
path, classesExtractionPath.resolve(srcWebInfClasses.relativize(path)));
AbsoluteUnixPath pathInContainer =
classesExtractionPath.resolve(webInfClasses.relativize(path));

if (path.getFileName().toString().endsWith(".class")) {
layerBuilder.addClassFile(path, pathInContainer);

} else if (Files.isDirectory(path)) {
try (Stream<Path> dirStream = Files.list(path)) {
if (!dirStream.findAny().isPresent()) {
try (Stream<Path> fileStream = Files.list(path)) {
if (!fileStream.findAny().isPresent()) {
// The directory is empty
layerBuilder.addResourceFile(
path, classesExtractionPath.resolve(srcWebInfClasses.relativize(path)));
layerBuilder.addResourceFile(path, pathInContainer);
}
}
} else {
layerBuilder.addResourceFile(
path, classesExtractionPath.resolve(srcWebInfClasses.relativize(path)));
layerBuilder.addResourceFile(path, pathInContainer);
}
});

Expand Down
1 change: 1 addition & 0 deletions jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/JibExtension.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ public JibExtension(Project project) {
allowInsecureRegistries = objectFactory.property(Boolean.class);
extraDirectory = objectFactory.property(Path.class);

// Sets defaults.
jvmFlags.set(Collections.emptyList());
args.set(Collections.emptyList());
useOnlyProjectCache.set(DEFAULT_USE_ONLY_PROJECT_CACHE);
Expand Down
30 changes: 13 additions & 17 deletions jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/JibPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

package com.google.cloud.tools.jib.gradle;

import com.google.cloud.tools.jib.frontend.JavaLayerConfigurations;
import com.google.common.annotations.VisibleForTesting;
import java.util.List;
import java.util.stream.Collectors;
Expand Down Expand Up @@ -44,18 +45,13 @@ public class JibPlugin implements Plugin<Project> {
@VisibleForTesting static final String EXPLODED_WAR_TASK_NAME = "jibExplodedWar";

@VisibleForTesting static final String DEFAULT_FROM_IMAGE = "gcr.io/distroless/java";
@VisibleForTesting static final String DEFAULT_WEBAPP_FROM_IMAGE = "gcr.io/distroless/java/jetty";
/**
* The default app root in the image. For example, if this is set to {@code "/app"}, dependency
* JARs will be in {@code "/app/libs"}.
*/
@VisibleForTesting static final String DEFAULT_APP_ROOT = "/app";
@VisibleForTesting static final String DEFAULT_WAR_FROM_IMAGE = "gcr.io/distroless/java/jetty";

/**
* The default webapp root in the image. For example, if this is set to {@code
* "/jetty/webapps/ROOT"}, dependency JARs will be in {@code "/jetty/webapps/ROOT/WEB-INF/lib"}.
* The default app root in the image for WAR. For example, if this is set to {@code
* /jetty/webapps/ROOT}, dependency JARs will be in {@code /jetty/webapps/ROOT/WEB-INF/lib}.
*/
@VisibleForTesting static final String DEFAULT_WEBAPP_ROOT = "/jetty/webapps/ROOT";
@VisibleForTesting static final String DEFAULT_WEB_APP_ROOT = "/jetty/webapps/ROOT";

/**
* Collects all project dependencies of the style "compile project(':mylib')" for any kind of
Expand Down Expand Up @@ -124,36 +120,36 @@ public void apply(Project project) {

project.afterEvaluate(
projectAfterEvaluation -> {
// TODO move this to a seperate place
// TODO move this to a separate place
try {
War warTask = GradleProjectProperties.getWarTask(project);
final Task dependsOnTask;
Task dependsOnTask;
if (warTask != null) {
if (jibExtension.getFrom().getImage() == null) {
jibExtension.getFrom().setImage(DEFAULT_WEBAPP_FROM_IMAGE);
jibExtension.getFrom().setImage(DEFAULT_WAR_FROM_IMAGE);
}
if (jibExtension.getContainer().getAppRoot().isEmpty()) {
jibExtension.getContainer().setAppRoot(DEFAULT_WEBAPP_ROOT);
jibExtension.getContainer().setAppRoot(DEFAULT_WEB_APP_ROOT);
}
// Has all tasks depend on the 'exploded war' task.
ExplodedWarTask explodedWarTask =
(ExplodedWarTask)
project
.getTasks()
.create(EXPLODED_WAR_TASK_NAME, ExplodedWarTask.class)
.dependsOn(warTask);
explodedWarTask.setWarFile(warTask.getArchivePath());
explodedWarTask.setWarFile(warTask.getArchivePath().toPath());
explodedWarTask.setExplodedWarDirectory(
GradleProjectProperties.getExplodedWarDirectory(projectAfterEvaluation));
// Have all tasks depend on the 'jibExplodedWar' task.
dependsOnTask = explodedWarTask;
} else {
if (jibExtension.getFrom().getImage() == null) {
jibExtension.getFrom().setImage(DEFAULT_FROM_IMAGE);
}
if (jibExtension.getContainer().getAppRoot().isEmpty()) {
jibExtension.getContainer().setAppRoot(DEFAULT_APP_ROOT);
jibExtension.getContainer().setAppRoot(JavaLayerConfigurations.DEFAULT_APP_ROOT);
}
// Has all tasks depend on the 'classes' task.
// Have all tasks depend on the 'classes' task.
dependsOnTask = projectAfterEvaluation.getTasks().getByPath("classes");
}
buildImageTask.dependsOn(dependsOnTask);
Expand Down
Loading