Update maven extraDirectory config parameter to include path and permissions field #1195

TadCordle · 2018-10-25T18:12:41Z

Fixes #794.

Adds/changes the <extraDirectory> configuration:

Can still configure extra directory using <extraDirectory>${project.basedir}/path/to/extra/directory</extraDirectory>
Can also configure the extra directory/permissions using new <extraDirector><path> and <extraDirectory><permissions> fields

e.g.

<configuration>
  ...
  <extraDirectory>
    <path>${project.basedir}/src/main/jib-custom</path>
    <permissions>
      <permission>
        <file>/foo</file>
        <mode>755</mode>
      </permission>
      <permission>
        <file>/bar/cat</file>
        <mode>777</mode>
      </permission>
    </permissions>
  </extraDirectory>
</configuration>

briandealwis

Would it be useful to support globs?

<permissions>
               <permission>
                 <file>/*.txrt</file>
                 <mode>755</mode>
               </permission>
               <permission>
                 <file>/**.sh</file>
                 <mode>555</mode>
               </permission>
             </permissions>

briandealwis · 2018-10-26T15:44:28Z

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

+  Path getExtraDirectoryPath() {
+    // TODO: Should inform user about nonexistent directory if using custom directory.
+    if (System.getProperty(PropertyNames.EXTRA_DIRECTORY_PATH) != null) {
+      return Paths.get(System.getProperty(PropertyNames.EXTRA_DIRECTORY_PATH));


Should check the Maven settings and pom properties too to keep with the semantics of Maven's @Parameter(property) values?

Is this a trivial change? It sounds like something we may want to do for all of the parameters, so maybe it should wait until another PR.

Speaking of which, I think supporting globs would be a good idea, but I think that should wait until another PR as well.

Filed #1200 and #1201 for these 2 comments.

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

coollog · 2018-10-26T05:21:02Z

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

+      this.mode = mode;
+    }
+
+    @Nullable


nit: let's start using Optional over @Nullable returns for all new code

coollog · 2018-10-26T05:22:46Z

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

-  // this parameter is cloned in FilesMojo
-  @Nullable
-  @Parameter(
-      defaultValue = "${project.basedir}/src/main/jib",


Should we have this default on ExtraDirectoryParameters rather than resolved via PluginConfigurationProcessor#getExtraDirectoryPath?

When I tried this I had trouble getting it to work in the nested class, but I've adjusted a lot since then so maybe I can try again.

Yeah, so the issue I have here is, it seems @Parameter(defaultValue = "...") doesn't work in nested classes, and I can't set the value on the same line as the variable declaration because I need project to get the base directory, which is out of scope.

Hmm, would passing project.basedir/src/main/jib in via a constructor work? And call new ExtraDirectoryParameters(project.basedir/src/main/jib) (not right syntax) on line 266.

I don't think so, since Maven requires/uses a default constructor. I'll try it though.

Edit: Yeah not looking promising.

I thought that was just for objects that Maven has to create (like objects in a list), but extraDirectory we are just constructing that explicitly?

From the looks of it, project is still null at the time we explicitly construct extraDirectory.

coollog · 2018-10-26T05:31:18Z

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

-      MavenProject project, Path extraDirectory, AbsoluteUnixPath appRoot) throws IOException {
+      MavenProject project,
+      Path extraDirectory,
+      Map<AbsoluteUnixPath, FilePermissions> permissions,


I think we should name this extraDirectoryPermissions or something similar since they only apply to the extra files.

jib-maven-plugin/CHANGELOG.md

chanseokoh · 2018-10-25T20:37:42Z

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

@@ -122,7 +142,7 @@ private static JavaLayerConfigurations getForWarProject(
        Paths.get(project.getBuild().getDirectory()).resolve(project.getBuild().getFinalName());
    // TODO: Replace Collections.emptyMap() with configured permissions


Remove the comment.

chanseokoh · 2018-10-26T18:32:25Z

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

+  @VisibleForTesting
+  static Map<AbsoluteUnixPath, FilePermissions> convertPermissionsMap(
+      Map<String, String> inputMap) {
+    ImmutableMap.Builder<AbsoluteUnixPath, FilePermissions> permissionsMap = ImmutableMap.builder();


nit: I think we can just return a HashMap. I see no need to prevent callers from modifying the returned map.

Oh yea, and for cases where we were to return ImmutableMap, we should be making the return type be ImmutableMap.

chanseokoh · 2018-10-26T18:35:47Z

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

+  @VisibleForTesting
+  static Map<AbsoluteUnixPath, FilePermissions> convertPermissionsList(
+      List<PermissionConfiguration> inputList) {
+    ImmutableMap.Builder<AbsoluteUnixPath, FilePermissions> permissionsMap = ImmutableMap.builder();


ditto, HashMap?

chanseokoh · 2018-10-29T16:23:35Z

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

+    return jibPluginConfiguration
+        .getExtraDirectoryPath()
+        .orElseGet(
+            () ->


I think orElse(Preconditions...) will work.

coollog

LGTM, just some small nits

coollog · 2018-10-29T17:20:07Z

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

@@ -202,12 +202,12 @@ static FileCollection getInputFiles(File extraDirectory, Project project) {
  @VisibleForTesting
  static Map<AbsoluteUnixPath, FilePermissions> convertPermissionsMap(
      Map<String, String> inputMap) {
-    ImmutableMap.Builder<AbsoluteUnixPath, FilePermissions> permissionsMap = ImmutableMap.builder();
+    HashMap<AbsoluteUnixPath, FilePermissions> permissionsMap = new HashMap<>();


tiny nit: prefer Map for type

coollog · 2018-10-29T17:27:49Z

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

+   * Validates and converts a {@code String->String} file-path-to-file-permissions map to an
+   * equivalent {@code AbsoluteUnixPath->FilePermission} map.
+   *
+   * @param inputMap the map to convert


nit: inputMap should probably be more descriptive - might be good to even include an example of an entry in this map.

coollog · 2018-10-29T17:29:33Z

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

+  public static class PermissionConfiguration {
+
+    @Nullable @Parameter private String file;
+


tiny nit: don't need to separate these with newline

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

coollog · 2018-10-29T17:34:51Z

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

-  // this parameter is cloned in FilesMojo
-  @Nullable
-  @Parameter(
-      defaultValue = "${project.basedir}/src/main/jib",


I thought that was just for objects that Maven has to create (like objects in a list), but extraDirectory we are just constructing that explicitly?

coollog · 2018-10-29T17:36:17Z

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

+   * Validates and converts a list of {@link PermissionConfiguration} to an equivalent {@code
+   * AbsoluteUnixPath->FilePermission} map.
+   *
+   * @param inputList the list to convert


ditto the variable naming nit

coollog · 2018-10-29T17:36:23Z

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

+  @VisibleForTesting
+  static Map<AbsoluteUnixPath, FilePermissions> convertPermissionsList(
+      List<PermissionConfiguration> inputList) {
+    HashMap<AbsoluteUnixPath, FilePermissions> permissionsMap = new HashMap<>();


ditto type nit

TadCordle added 3 commits October 25, 2018 11:45

Tests pass

3ba55a7

Actually works

7396e3a

Works now

c1b5dd9

googlebot added the cla: yes label Oct 25, 2018

TadCordle requested review from coollog, loosebazooka, chanseokoh and briandealwis October 25, 2018 18:13

TadCordle added 5 commits October 25, 2018 14:18

Format

3e8d3f3

Move around/improve validation

011ce4e

Fix javadoc

c8a77d2

Improve error message

0f362b6

Format

2e02c22

briandealwis reviewed Oct 26, 2018

View reviewed changes

coollog reviewed Oct 26, 2018

View reviewed changes

chanseokoh reviewed Oct 26, 2018

View reviewed changes

TadCordle added 2 commits October 29, 2018 10:45

Feedback

28ebf99

More Optional

75efd6e

This was referenced Oct 29, 2018

Support globs in file permission configuration #1200

Closed

Check Maven settings and pom properties as well as system properties in maven config #1201

Closed

chanseokoh reviewed Oct 29, 2018

View reviewed changes

orElse

26f1185

coollog approved these changes Oct 29, 2018

View reviewed changes

Feedback

30cf778

TadCordle merged commit 0174a49 into master Oct 29, 2018

TadCordle deleted the file-permissions-maven branch October 29, 2018 18:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update maven extraDirectory config parameter to include path and permissions field #1195

Update maven extraDirectory config parameter to include path and permissions field #1195

TadCordle commented Oct 25, 2018 •

edited

Loading

briandealwis left a comment

briandealwis Oct 26, 2018

TadCordle Oct 26, 2018

TadCordle Oct 26, 2018

TadCordle Oct 29, 2018

coollog Oct 26, 2018

coollog Oct 26, 2018

TadCordle Oct 26, 2018

TadCordle Oct 29, 2018

coollog Oct 29, 2018

TadCordle Oct 29, 2018 •

edited

Loading

coollog Oct 29, 2018

TadCordle Oct 29, 2018

coollog Oct 26, 2018

chanseokoh Oct 25, 2018

chanseokoh Oct 26, 2018

coollog Oct 26, 2018

chanseokoh Oct 26, 2018

chanseokoh Oct 29, 2018

coollog left a comment

coollog Oct 29, 2018

coollog Oct 29, 2018

coollog Oct 29, 2018

coollog Oct 29, 2018

coollog Oct 29, 2018

coollog Oct 29, 2018

		@@ -122,7 +142,7 @@ private static JavaLayerConfigurations getForWarProject(
		Paths.get(project.getBuild().getDirectory()).resolve(project.getBuild().getFinalName());
		// TODO: Replace Collections.emptyMap() with configured permissions

		public static class PermissionConfiguration {

		@Nullable @Parameter private String file;

Update maven extraDirectory config parameter to include path and permissions field #1195

Update maven extraDirectory config parameter to include path and permissions field #1195

Conversation

TadCordle commented Oct 25, 2018 • edited Loading

briandealwis left a 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

TadCordle Oct 29, 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

coollog left a 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

TadCordle commented Oct 25, 2018 •

edited

Loading

TadCordle Oct 29, 2018 •

edited

Loading