Fix review comments

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

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

@@ -135,7 +135,7 @@ public static class ContainerParameters {
 
     @Parameter private Map<String, String> labels = Collections.emptyMap();
 
-    @Nullable @Parameter private String appRoot;
+    @Parameter private String appRoot = "";
   }
 
   @Nullable
@@ -249,7 +249,6 @@ Map<String, String> getLabels() {
     return container.labels;
   }
 
-  @Nullable
   String getAppRoot() {
     return container.appRoot;
   }

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

@@ -34,8 +34,6 @@
 
 /** Builds {@link JavaLayerConfigurations} based on inputs from a {@link MavenProject}. */
 class MavenLayerConfigurations {
-  public static final String CLASS_EXTENSION = ".class";
-
   /**
    * Resolves the {@link JavaLayerConfigurations} for a {@link MavenProject}.
    *
@@ -90,7 +88,7 @@ private static JavaLayerConfigurations getForNonWarProject(
     Path classesOutputDirectory = Paths.get(project.getBuild().getOutputDirectory());
 
     // Gets the classes files in the 'classes' output directory.
-    Predicate<Path> isClassFile = path -> path.toString().endsWith(CLASS_EXTENSION);
+    Predicate<Path> isClassFile = path -> path.toString().endsWith(".class");
     addFilesToLayer(
         classesOutputDirectory, isClassFile, classesExtractionPath, layerBuilder::addClassFile);
 
@@ -124,7 +122,8 @@ private static JavaLayerConfigurations getForWarProject(
 
     // TODO explode the WAR file rather than using this directory. The contents of the final WAR may
     // be different from this directory (it's possible to include or exclude files when packaging a
-    // WAR).
+    // WAR). Also the exploded WAR directory is configurable with and may not be at
+    // build.getFinalName().
     Path explodedWarPath =
         Paths.get(project.getBuild().getDirectory()).resolve(project.getBuild().getFinalName());
     AbsoluteUnixPath dependenciesExtractionPath = appRoot.resolve("WEB-INF/lib/");
@@ -149,7 +148,7 @@ private static JavaLayerConfigurations getForWarProject(
     }
 
     // Gets the classes files in the 'WEB-INF/classes' output directory.
-    Predicate<Path> isClassFile = path -> path.getFileName().toString().endsWith(CLASS_EXTENSION);
+    Predicate<Path> isClassFile = path -> path.getFileName().toString().endsWith(".class");
     if (Files.exists(explodedWarPath.resolve("WEB-INF/classes"))) {
       addFilesToLayer(
           explodedWarPath.resolve("WEB-INF/classes/"),

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

@@ -40,12 +40,20 @@
 
 /** Configures and provides builders for the image building goals. */
 class PluginConfigurationProcessor {
+  /**
+   * Returns true if the maven packaging type is "war"
+   *
+   * @param jibPluginConfiguration the Jib plugin configuration
+   * @return true if the maven packaging type is "war"
+   */
+  private static boolean isWarPackaging(JibPluginConfiguration jibPluginConfiguration) {
+    return "war".equals(jibPluginConfiguration.getProject().getPackaging());
+  }
 
   /**
-   * Gets the value of the {@code <container><appRoot>} parameter. Throws {@link
-   * MojoExecutionException} if it is not an absolute path in Unix-style. If this parameter is null,
-   * {@link JavaLayerConfigurations#DEFAULT_WEB_APP_ROOT} is returned for WAR packaging, {@link
-   * JavaLayerConfigurations#DEFAULT_APP_ROOT} is returned otherwise.
+   * Gets the value of the {@code <container><appRoot>} parameter. If the parameter is empty,
+   * returns {@link JavaLayerConfigurations#DEFAULT_WEB_APP_ROOT} for project with WAR packaging or
+   * {@link JavaLayerConfigurations#DEFAULT_APP_ROOT} for other packaging.
    *
    * @param jibPluginConfiguration the Jib plugin configuration
    * @return the app root value
@@ -54,7 +62,7 @@ class PluginConfigurationProcessor {
   static AbsoluteUnixPath getAppRootChecked(JibPluginConfiguration jibPluginConfiguration)
       throws MojoExecutionException {
     String appRoot = jibPluginConfiguration.getAppRoot();
-    if (appRoot == null) {
+    if (appRoot.isEmpty()) {
       appRoot =
           isWarPackaging(jibPluginConfiguration)
               ? JavaLayerConfigurations.DEFAULT_WEB_APP_ROOT
@@ -69,34 +77,23 @@ static AbsoluteUnixPath getAppRootChecked(JibPluginConfiguration jibPluginConfig
   }
 
   /**
-   * Gets the value of the {@code <to><image>} parameter. If this parameter is null,
-   * "gcr.io/distroless/java/jetty" is returned for war packaging, "gcr.io/distroless/java" is
-   * returned otherwise.
+   * Gets the value of the {@code <from><image>} parameter. If the parameter is null, returns
+   * "gcr.io/distroless/java/jetty" for projects with WAR packaging or "gcr.io/distroless/java" for
+   * other packaging.
    *
    * @param jibPluginConfiguration the Jib plugin configuration
    * @return the base image value
    */
   static String getBaseImage(JibPluginConfiguration jibPluginConfiguration) {
     String baseImage = jibPluginConfiguration.getBaseImage();
     if (baseImage == null) {
-      baseImage =
-          isWarPackaging(jibPluginConfiguration)
-              ? "gcr.io/distroless/java/jetty"
-              : "gcr.io/distroless/java";
+      return isWarPackaging(jibPluginConfiguration)
+          ? "gcr.io/distroless/java/jetty"
+          : "gcr.io/distroless/java";
     }
     return baseImage;
   }
 
-  /**
-   * Returns true if the maven packaging type is "war"
-   *
-   * @param jibPluginConfiguration the Jib plugin configuration
-   * @return true if the maven packaging type is "war"
-   */
-  private static boolean isWarPackaging(JibPluginConfiguration jibPluginConfiguration) {
-    return "war".equals(jibPluginConfiguration.getProject().getPackaging());
-  }
-
   /** Disables annoying Apache HTTP client logging. */
   static void disableHttpLogging() {
     System.setProperty(
@@ -219,6 +216,42 @@ static ImageReference parseImageReference(String image, String type) {
     }
   }
 
+  /**
+   * Compute the container entrypoint, in this order :
+   *
+   * <ol>
+   *   <li>the user specified one, if set
+   *   <li>for a war project, the jetty default one
+   *   <li>for a jar project, by resolving the main class
+   * </ol>
+   *
+   * @param logger the logger used to display messages.
+   * @param jibPluginConfiguration the {@link JibPluginConfiguration} providing the configuration
+   *     data
+   * @param projectProperties used for providing additional information
+   * @return the entrypoint
+   * @throws MojoExecutionException if the http timeout system property is misconfigured
+   */
+  static List<String> computeEntrypoint(
+      Log logger,
+      JibPluginConfiguration jibPluginConfiguration,
+      MavenProjectProperties projectProperties)
+      throws MojoExecutionException {
+    if (!jibPluginConfiguration.getEntrypoint().isEmpty()) {
+      if (jibPluginConfiguration.getMainClass() != null
+          || !jibPluginConfiguration.getJvmFlags().isEmpty()) {
+        logger.warn("<mainClass> and <jvmFlags> are ignored when <entrypoint> is specified");
+      }
+      return jibPluginConfiguration.getEntrypoint();
+    }
+    if (isWarPackaging(jibPluginConfiguration)) {
+      return JavaEntrypointConstructor.makeDistrolessJettyEntrypoint();
+    }
+    String mainClass = projectProperties.getMainClass(jibPluginConfiguration);
+    return JavaEntrypointConstructor.makeDefaultEntrypoint(
+        getAppRootChecked(jibPluginConfiguration), jibPluginConfiguration.getJvmFlags(), mainClass);
+  }
+
   private final BuildConfiguration.Builder buildConfigurationBuilder;
   private final ImageConfiguration.Builder baseImageConfigurationBuilder;
   private final ContainerConfiguration.Builder containerConfigurationBuilder;
@@ -257,41 +290,4 @@ MavenSettingsServerCredentials getMavenSettingsServerCredentials() {
   boolean isBaseImageCredentialPresent() {
     return isBaseImageCredentialPresent;
   }
-
-  /**
-   * Compute the container entrypoint, in this order :
-   *
-   * <ol>
-   *   <li>the user specified one, if set
-   *   <li>for a war project, the jetty default one
-   *   <li>for a jar project, by resolving the main class
-   * </ol>
-   *
-   * @param logger the logger used to display messages.
-   * @param jibPluginConfiguration the {@link JibPluginConfiguration} providing the configuration
-   *     data
-   * @param projectProperties used for providing additional information
-   * @return the entrypoint
-   * @throws MojoExecutionException if the http timeout system property is misconfigured
-   */
-  static List<String> computeEntrypoint(
-      Log logger,
-      JibPluginConfiguration jibPluginConfiguration,
-      MavenProjectProperties projectProperties)
-      throws MojoExecutionException {
-
-    if (!jibPluginConfiguration.getEntrypoint().isEmpty()) {
-      if (jibPluginConfiguration.getMainClass() != null
-          || !jibPluginConfiguration.getJvmFlags().isEmpty()) {
-        logger.warn("<mainClass> and <jvmFlags> are ignored when <entrypoint> is specified");
-      }
-      return jibPluginConfiguration.getEntrypoint();
-    }
-    if (isWarPackaging(jibPluginConfiguration)) {
-      return JavaEntrypointConstructor.makeDistrolessJettyEntrypoint();
-    }
-    String mainClass = projectProperties.getMainClass(jibPluginConfiguration);
-    return JavaEntrypointConstructor.makeDefaultEntrypoint(
-        getAppRootChecked(jibPluginConfiguration), jibPluginConfiguration.getJvmFlags(), mainClass);
-  }
 }

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.assertNull(testPluginConfiguration.getAppRoot());
+    Assert.assertEquals("", testPluginConfiguration.getAppRoot());
   }
 }

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

@@ -400,4 +400,52 @@ public void testGetForJarProject_nonDefaultAppRoot() throws URISyntaxException,
 
     assertNonDefaultAppRoot(configuration);
   }
+
+  @Test
+  public void testGetForWarProject_noErrorIfWebInfDoesNotExist()
+      throws IOException, URISyntaxException {
+    temporaryFolder.newFolder("final-name");
+    Mockito.when(mockMavenProject.getPackaging()).thenReturn("war");
+    Mockito.when(mockBuild.getDirectory())
+        .thenReturn(temporaryFolder.getRoot().toPath().toString());
+    Mockito.when(mockBuild.getFinalName()).thenReturn("final-name");
+    AbsoluteUnixPath appRoot = AbsoluteUnixPath.get("/my/app");
+
+    Path extraFilesDirectory = Paths.get(Resources.getResource("layer").toURI());
+
+    MavenLayerConfigurations.getForProject(
+        mockMavenProject, extraFilesDirectory, appRoot); // should pass
+  }
+
+  @Test
+  public void testGetForWarProject_noErrorIfWebInfLibDoesNotExist()
+      throws IOException, URISyntaxException {
+    temporaryFolder.newFolder("final-name", "WEB-INF", "classes");
+    Mockito.when(mockMavenProject.getPackaging()).thenReturn("war");
+    Mockito.when(mockBuild.getDirectory())
+        .thenReturn(temporaryFolder.getRoot().toPath().toString());
+    Mockito.when(mockBuild.getFinalName()).thenReturn("final-name");
+    AbsoluteUnixPath appRoot = AbsoluteUnixPath.get("/my/app");
+
+    Path extraFilesDirectory = Paths.get(Resources.getResource("layer").toURI());
+
+    MavenLayerConfigurations.getForProject(
+        mockMavenProject, extraFilesDirectory, appRoot); // should pass
+  }
+
+  @Test
+  public void testGetForWarProject_noErrorIfWebInfClassesDoesNotExist()
+      throws IOException, URISyntaxException {
+    temporaryFolder.newFolder("final-name", "WEB-INF", "lib");
+    Mockito.when(mockMavenProject.getPackaging()).thenReturn("war");
+    Mockito.when(mockBuild.getDirectory())
+        .thenReturn(temporaryFolder.getRoot().toPath().toString());
+    Mockito.when(mockBuild.getFinalName()).thenReturn("final-name");
+    AbsoluteUnixPath appRoot = AbsoluteUnixPath.get("/my/app");
+
+    Path extraFilesDirectory = Paths.get(Resources.getResource("layer").toURI());
+
+    MavenLayerConfigurations.getForProject(
+        mockMavenProject, extraFilesDirectory, appRoot); // should pass
+  }
 }

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

@@ -251,7 +251,7 @@ public void testGetAppRootChecked_errorOnWindowsPathWithDriveLetter() {
 
   @Test
   public void testGetAppRootChecked_defaultNonWarPackaging() throws MojoExecutionException {
-    Mockito.doReturn(null).when(mockJibPluginConfiguration).getAppRoot();
+    Mockito.doReturn("").when(mockJibPluginConfiguration).getAppRoot();
     Mockito.doReturn(mavenProject).when(mockJibPluginConfiguration).getProject();
     Mockito.doReturn(null).when(mavenProject).getPackaging();
 
@@ -262,7 +262,7 @@ public void testGetAppRootChecked_defaultNonWarPackaging() throws MojoExecutionE
 
   @Test
   public void testGetAppRootChecked_defaultJarPackaging() throws MojoExecutionException {
-    Mockito.doReturn(null).when(mockJibPluginConfiguration).getAppRoot();
+    Mockito.doReturn("").when(mockJibPluginConfiguration).getAppRoot();
     Mockito.doReturn(mavenProject).when(mockJibPluginConfiguration).getProject();
     Mockito.doReturn("jar").when(mavenProject).getPackaging();
 
@@ -273,7 +273,7 @@ public void testGetAppRootChecked_defaultJarPackaging() throws MojoExecutionExce
 
   @Test
   public void testGetAppRootChecked_defaultWarPackaging() throws MojoExecutionException {
-    Mockito.doReturn(null).when(mockJibPluginConfiguration).getAppRoot();
+    Mockito.doReturn("").when(mockJibPluginConfiguration).getAppRoot();
     Mockito.doReturn(mavenProject).when(mockJibPluginConfiguration).getProject();
     Mockito.doReturn("war").when(mavenProject).getPackaging();