Maven War support #1068

CyrilleH · 2018-09-29T15:09:30Z

Towards #431, here's my proposal to support "WAR" project with Jib and Maven.

This PR is based from #923.
Once #923 is merged, I will rebase on master. So, please review only the last commit.

chanseokoh

Awesome, @CyrilleH. Thanks a lot for your continued non-trivial contributions! Looks generally good at a high level, and a few things I found in my first quick pass.

chanseokoh · 2018-10-01T14:53:15Z

jib-core/src/main/java/com/google/cloud/tools/jib/frontend/JavaLayerConfigurations.java

+  /** The standard directory name containing libs and snapshot-libs in a War Project */
+  public static final String WEB_INF_LIB_RELATIVE_PATH = WEB_INF_RELATIVE_PATH + "/lib/";
+  /** The standard directory name containing classes and some resources in a War Project */
+  public static final String WEB_INF_CLASSES_RELATIVE_PATH = WEB_INF_RELATIVE_PATH + "/classes/";


I think just embedding these constants (except for DEFAULT_WEB_APP_ROOT) has more benefits in this case than declaring as fields here. (#1072 (comment))

BTW, WEBAPP--> WEB_APP

chanseokoh · 2018-10-01T15:00:24Z

jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/MavenLayerConfigurations.java

@@ -46,6 +47,25 @@
   */
  static JavaLayerConfigurations getForProject(
      MavenProject project, Path extraDirectory, AbsoluteUnixPath appRoot) throws IOException {
+    // Since 'jar' is the default packaging type it is not required, so it can be null


It's OK to do "war".equals(project.getPackaging()) here and remove the null check and the comment.

jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/MavenLayerConfigurations.java

chanseokoh · 2018-10-01T15:04:04Z

jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/MavenLayerConfigurations.java

+        layerBuilder::addDependencyFile);
+
+    // Gets the classes files in the 'WEB-INF/classes' output directory.
+    Predicate<Path> isClassFile = path -> path.toString().endsWith(CLASS_EXTENSION);


Now I'm thinking path.getFileName().toString() is safer.

chanseokoh · 2018-10-01T15:34:48Z

jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/MavenLayerConfigurations.java

+          boolean inWebInfLib =
+              path.startsWith(
+                  explodedWarPath.resolve(JavaLayerConfigurations.WEB_INF_LIB_RELATIVE_PATH));
+          boolean isClass = path.toString().endsWith(CLASS_EXTENSION);


IsClassFile above, so no need here.

~~Maybe we can have this predicate as a static class method and share it with getForProject too.~~ On second thought, a static method might be an overkill for this simple code.

chanseokoh · 2018-10-01T15:47:51Z

...aven-plugin/src/main/java/com/google/cloud/tools/jib/maven/PluginConfigurationProcessor.java

+      MavenProjectProperties projectProperties)
+      throws MojoExecutionException {
+
+    List<String> entrypoint = jibPluginConfiguration.getEntrypoint();


See the changes here which I think is more readable.

chanseokoh · 2018-10-01T15:48:52Z

jib-maven-plugin/src/test/java/com/google/cloud/tools/jib/maven/DockerContextMojoTest.java

@@ -41,13 +43,13 @@

  private DockerContextMojo mojo;
  private String appRoot = "/app";
+  private File outputFolder;
+  @Mock MavenProject project;
+  @Mock Build build;


private for these two fields.

chanseokoh · 2018-10-01T15:50:28Z

jib-maven-plugin/src/test/java/com/google/cloud/tools/jib/maven/JibPluginConfigurationTest.java

@@ -48,6 +48,6 @@ public void testAuthDefaults() {
    Assert.assertEquals(
        "<to><auth><password>",
        testPluginConfiguration.getTargetImageAuth().getPasswordPropertyDescriptor());
-    Assert.assertEquals("/app", testPluginConfiguration.getAppRoot());
+    Assert.assertEquals(null, testPluginConfiguration.getAppRoot());


Assert.assertNull

chanseokoh · 2018-10-01T15:50:51Z

...aven-plugin/src/test/java/com/google/cloud/tools/jib/maven/MavenLayerConfigurationsTest.java

-        configuration.getClassLayerEntries());
-    assertExtractionPathsUnordered(
-        Arrays.asList("/a/b/bar", "/c/cat", "/foo"), configuration.getExtraFilesLayerEntries());
+    assert_nonDefaultAppRoot(configuration);


assertNonDefaultAppRoot

chanseokoh · 2018-10-01T15:52:18Z

...aven-plugin/src/test/java/com/google/cloud/tools/jib/maven/MavenLayerConfigurationsTest.java

+    assert_nonDefaultAppRoot(configuration);
+  }
+
+  private void assert_nonDefaultAppRoot(JavaLayerConfigurations configuration) {


Can this be private static? If so, change it to static and move the method code above any non-static methods or fields.

chanseokoh · 2018-10-01T17:43:33Z

jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/MavenLayerConfigurations.java

+      MavenProject project, Path extraDirectory, AbsoluteUnixPath appRoot) throws IOException {
+
+    Path explodedWarPath =
+        Paths.get(project.getBuild().getDirectory()).resolve(project.getBuild().getFinalName());


Also, I think getFinalName() may not work (admittedly in rare cases), because the exploded WAR directory is configurable with <webappDirectory> in the Maven WAR plugin, although the default is ${project.build.directory}/${project.build.finalName}. For example,

<plugin> <groupId>org.apache.maven.plugins</groupId> <artifactId>maven-war-plugin</artifactId> <version>3.2.2</version> <configuration> <webappDirectory>${project.build.directory}/my-webapp</webappDirectory> </configuration> </plugin>

$ ls target classes/ generated-sources/ helloworld-1.war maven-archiver/ maven-status/ my-webapp/

But since we will eventually unzip the WAR file, I think it's OK for now to use finalName().

chanseokoh · 2018-10-01T20:30:13Z

...gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/GradleLayerConfigurations.java

+    // Some files in WEB-INF need to be in resources directory (e.g. web.xml, ...)
+    Path webinfOutputDirectory =
+        explodedWarPath.resolve(JavaLayerConfigurations.WEB_INF_RELATIVE_PATH);
+    try (Stream<Path> fileStream = Files.list(webinfOutputDirectory)) {


We also need to check the directory existence for WEB-INF, WEB-INF/lib, and WEB-INF/classes. (See #1074.)

CyrilleH · 2018-10-01T22:48:26Z

Since #923 is merged, I have rebased on master.

I have to add some unit tests to check it works correctly if "WEB-INF" is not present (and "WEB-INF/lib" and "WEB-INF/classes").
Sorry for this mistake.

chanseokoh · 2018-10-02T13:56:15Z

No problem. We can always fix bugs later.

chanseokoh · 2018-10-02T15:38:02Z

jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/JibPluginConfiguration.java

@@ -139,7 +135,7 @@ public void set(String image) {

    @Parameter private Map<String, String> labels = Collections.emptyMap();

-    @Parameter private String appRoot = JavaLayerConfigurations.DEFAULT_APP_ROOT;
+    @Nullable @Parameter private String appRoot;


I remember the Gradle counterpart set this to an empty string ("") instead of null. Can we be consistent? is there some reason they cannot be consistent? Not sure whether an empty string or null is better though.

jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/JibPluginConfiguration.java

chanseokoh · 2018-10-02T15:41:29Z

jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/MavenLayerConfigurations.java

@@ -71,7 +90,7 @@ static JavaLayerConfigurations getForProject(
    Path classesOutputDirectory = Paths.get(project.getBuild().getOutputDirectory());

    // Gets the classes files in the 'classes' output directory.
-    Predicate<Path> isClassFile = path -> path.toString().endsWith(".class");
+    Predicate<Path> isClassFile = path -> path.toString().endsWith(CLASS_EXTENSION);


nit: I'm fine with embedding the string .class.

jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/MavenLayerConfigurations.java

chanseokoh · 2018-10-02T15:48:16Z

...aven-plugin/src/main/java/com/google/cloud/tools/jib/maven/PluginConfigurationProcessor.java

+   * @param jibPluginConfiguration the Jib plugin configuration
+   * @return true if the maven packaging type is "war"
+   */
+  private static boolean isWarPackaging(JibPluginConfiguration jibPluginConfiguration) {


style nit: in our codebase, private static methods go below (package-private) static methods.

chanseokoh · 2018-10-02T15:49:52Z

...aven-plugin/src/main/java/com/google/cloud/tools/jib/maven/PluginConfigurationProcessor.java

+   * @return the entrypoint
+   * @throws MojoExecutionException if the http timeout system property is misconfigured
+   */
+  static List<String> computeEntrypoint(


style nit: static methods go above non-static fields or methods.

jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/MavenLayerConfigurations.java

...aven-plugin/src/main/java/com/google/cloud/tools/jib/maven/PluginConfigurationProcessor.java

jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/JibPluginConfiguration.java

jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/MavenLayerConfigurations.java

...aven-plugin/src/main/java/com/google/cloud/tools/jib/maven/PluginConfigurationProcessor.java

chanseokoh · 2018-10-03T16:21:28Z

Thanks, @CyrilleH. I think this looks good. Merging with master, fixing one or two nits, and I think we're good to go.

Adds unit tests on WEB-INF, WEB-INF/classes, WEB-INF/lib existence

Fix comment

CyrilleH · 2018-10-03T20:13:25Z

Great, @chanseokoh
I rebased my branch on master.
Please tell me if I missed something or if I have to squash the branch into one commit.

chanseokoh

Awesome! I think we can merge this. I can take care of some very minor cleanups. Thanks again!

googlebot added the cla: yes label Sep 29, 2018

CyrilleH force-pushed the maven-war-support branch from 352ebba to cf09041 Compare September 30, 2018 13:57

CyrilleH mentioned this pull request Sep 30, 2018

Inherit ENTRYPOINT and CMD from base image for WAR projects #1071

Merged

CyrilleH changed the title ~~Maven war support~~ Maven War support Sep 30, 2018

chanseokoh reviewed Oct 1, 2018

View reviewed changes

chanseokoh added the kokoro:run label Oct 1, 2018

kokoro-team removed the kokoro:run label Oct 1, 2018

chanseokoh reviewed Oct 1, 2018

View reviewed changes

chanseokoh mentioned this pull request Oct 1, 2018

Support WARs. #431

Closed

chanseokoh reviewed Oct 1, 2018

View reviewed changes

CyrilleH force-pushed the maven-war-support branch from cf09041 to 122b42b Compare October 1, 2018 22:10

chanseokoh added the kokoro:run label Oct 2, 2018

kokoro-team removed the kokoro:run label Oct 2, 2018

chanseokoh reviewed Oct 2, 2018

View reviewed changes

chanseokoh requested a review from a team October 2, 2018 19:19

coollog reviewed Oct 2, 2018

View reviewed changes

chanseokoh mentioned this pull request Oct 2, 2018

Consider removing ProjectProperties.isWarProject() #1088

Closed

CyrilleH force-pushed the maven-war-support branch 2 times, most recently from c65ac19 to 3b6dc08 Compare October 3, 2018 07:14

chanseokoh added the kokoro:run label Oct 3, 2018

kokoro-team removed the kokoro:run label Oct 3, 2018

chanseokoh reviewed Oct 3, 2018

View reviewed changes

chanseokoh mentioned this pull request Oct 3, 2018

Be consistent with evaluating Maven and Gradle parameter values #1090

Closed

CyrilleH added 4 commits October 3, 2018 21:52

Maven War Support

91c8912

Fix review comments

4717c65

Fix review comments

2a82e4d

Adds unit tests on WEB-INF, WEB-INF/classes, WEB-INF/lib existence

Fix conflict resolution error

25da9e4

Fix comment

CyrilleH force-pushed the maven-war-support branch from 3b6dc08 to 25da9e4 Compare October 3, 2018 20:09

chanseokoh added the kokoro:run label Oct 3, 2018

kokoro-team removed the kokoro:run label Oct 3, 2018

chanseokoh approved these changes Oct 3, 2018

View reviewed changes

chanseokoh merged commit 1171796 into GoogleContainerTools:master Oct 4, 2018

CyrilleH deleted the maven-war-support branch October 4, 2018 14:54

Maven War support #1068

Maven War support #1068

Conversation

CyrilleH commented Sep 29, 2018 • edited Loading

chanseokoh left a comment

Choose a reason for hiding this comment

chanseokoh Oct 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chanseokoh Oct 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chanseokoh Oct 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrilleH commented Oct 1, 2018 • edited Loading

chanseokoh commented Oct 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chanseokoh commented Oct 3, 2018

CyrilleH commented Oct 3, 2018

chanseokoh left a comment

Choose a reason for hiding this comment

CyrilleH commented Sep 29, 2018 •

edited

Loading

chanseokoh Oct 1, 2018 •

edited

Loading

chanseokoh Oct 1, 2018 •

edited

Loading

chanseokoh Oct 1, 2018 •

edited

Loading

CyrilleH commented Oct 1, 2018 •

edited

Loading