Add container.appRoot config parameter #984

chanseokoh · 2018-09-13T18:30:18Z

Fixes #964

loosebazooka · 2018-09-13T20:14:25Z

jib-core/src/main/java/com/google/cloud/tools/jib/configuration/ContainerConfiguration.java

@@ -32,6 +32,12 @@
  /** The default creation time of the container (constant to ensure reproducibility by default). */
  public static final Instant DEFAULT_CREATION_TIME = Instant.EPOCH;

+  /**
+   * The default app root in the image. For example, if this is set to {@code "/helloworld-app"},


I feel like the example should just use the current default value? #976

loosebazooka · 2018-09-13T20:15:08Z

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

@@ -131,6 +131,11 @@ private static String mapToDockerfileString(Map<String, String> map, String comm
    return joiner.toString();
  }

+  @VisibleForTesting
+  static String getUnixPath(Path path) {


I think another PR has a similar utility, might be worth using the same for both?

Certainly. Since that utility is not yet available, I'll add a TODO here to use the utility.

loosebazooka · 2018-09-13T20:16:27Z

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

  }

  /** Builds with each layer's files. */
  public static class Builder {

    private final Map<LayerType, List<Path>> layerFilesMap = new EnumMap<>(LayerType.class);
+    private Path appRoot = Paths.get(ContainerConfiguration.DEFAULT_APP_ROOT);


I'm worried path might be implementation specific (different on windows), and this should be a string? Specifically when running Jib on a windows machine.

I had the same concern intially, but since the existing code is actually using Path to pass to addEntryRecursively(), I think it seemed better to accept Path for now: https://github.com/GoogleContainerTools/jib/pull/984/files#diff-1105d9e04ba077f47449d5c7095d4252L123 Maybe addEntryRecursively() shouldn't accept Path.

Actually, your suggestion seems better. I changed it to accept Unix-style path in String instead of Path. This also removes the need for the toUnixPath() helper.

Looks like addEntryRecursively actually boils down to some Unix path conversion eventually.

jib/jib-core/src/main/java/com/google/cloud/tools/jib/image/LayerEntry.java

Line 126 in 8f14bd5

public String getAbsoluteExtractionPathString() {

Yeah, so I'm wondering all the methods and classes that need the extraction path info should get the path in String rather than Path to indicate clearly that the extraction path is and will end up in Unix-style.

loosebazooka · 2018-09-13T20:17:01Z

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

@@ -110,6 +125,8 @@ public Builder setExplodedWarFiles(List<Path> explodedWarFiles) {
    }

    public JavaLayerConfigurations build() throws IOException {
+      Preconditions.checkState(appRoot.isAbsolute(), "'appRoot' must be an absolute path");


this seems like we might need to use a custom validator that is not OS specific.

Not sure I get what you mean. (Is C:\dir not absolute, for example? Even so, I think it is right to fail, since we assume we build a Linux image.)

Anyways, I think we have been assuming that this appRoot should always in Unix-style in the end.

Whatever you meant, I think this will be taken care of properly after I appRoot from Path to String. The new condition will look like

// Windows filenames cannot have "/", so this also blocks Windows-style path. Preconditions.checkState(appRoot.startsWith("/"), "appRoot should be an absolute path in Unix-style");

loosebazooka · 2018-09-13T20:18:38Z

oops sorry I jumped the gun on the review there a little. Didn't see the NOT READY label.

…to i964-container.appRoot

chanseokoh · 2018-09-14T21:52:59Z

Ready for review.

You might first think too many files were touched, but the actual code change to make this work isn't really that large, when excluding tests.

chanseokoh · 2018-09-17T18:01:31Z

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

@@ -36,47 +36,62 @@
  /** Represents the different types of layers for a Java application. */
  @VisibleForTesting
  enum LayerType {
-    DEPENDENCIES("dependencies", JavaEntrypointConstructor.DEFAULT_DEPENDENCIES_PATH_ON_IMAGE),
+    DEPENDENCIES(


Thinking more about this enum where we need to dynamically decide the final extraction path, I'm now leaning toward getting rid of static extraction path initialization with the appRootRelative switch. Instead, the JavaLayerConfiguration.Builder() could accept the extraction path when setting layer files. That is, change setClassFiles(List<Path> classFiles) to setClassFiles(String extractionPath, List<Path> classFiles). That seems more straightforward for our dynamic path requirement and simpler. What do you think?

Yea that sounds good. One nit is to have it be setClassFiles(List<Path> classFiles, String extractionPath) to be more like LayerEntry.

chanseokoh · 2018-09-17T18:44:12Z

Will re-design this per #984 (comment). Not ready for review until then.

chanseokoh · 2018-09-17T23:15:50Z

Ready for review

jib-core/src/main/java/com/google/cloud/tools/jib/configuration/ContainerConfiguration.java

coollog · 2018-09-18T20:00:09Z

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

@@ -134,10 +146,13 @@ public static Builder builder() {
  }

  private final ImmutableMap<LayerType, LayerConfiguration> layerConfigurationMap;
+  private final ImmutableMap<LayerType, String> defaultExtractionPathMap;


Just extractionPathMap? This doesn't look like it's getting any defaults.

coollog · 2018-09-18T20:00:36Z

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

-    this.layerConfigurationMap = layerConfigurationsMap;
+      ImmutableMap<LayerType, LayerConfiguration> layerConfigurationsMap,
+      ImmutableMap<LayerType, String> defaultExtractionPathMap) {
+    layerConfigurationMap = layerConfigurationsMap;


Perhaps we should name these both layerConfigurationsMap so this can be the more familiar this.x = x form?

coollog · 2018-09-18T20:01:19Z

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

@@ -170,7 +185,35 @@ private JavaLayerConfigurations(
    return getLayerEntries(LayerType.EXPLODED_WAR);
  }

+  public String getDependencyDefaultExtractionPath() {


Are these getting defaults or the actual extraction path?

I'll drop Default from the name.

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

coollog · 2018-09-18T20:53:57Z

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


    private Builder() {
      for (LayerType layerType : LayerType.values()) {
        layerFilesMap.put(layerType, new ArrayList<>());
+        extractionPathMap.put(layerType, "/");


Is there a need to initialize these with the path as "/"? (Perhaps the code that builds the LayerConfiguration can skip if the files or extraction path is empty?)

I think it's good to have "/". Otherwise, we may go into the business of returning Optional<String> for getResourceExtractionPath() instead of String.

Hmm, since these get...ExtractionPath methods are really only used for Docker context generation (via addIfNotEmpty) and since this default / is not actually used when the layer entries is empty, perhaps we would need to actually reorganize the API of JavaLayerConfigurations, but this can be a separate issue.

That's a good idea. I was thinking the same. I'll file an issue.

Filed #1010.

coollog

Are you planning to merge the Absolute/RelativeUnixPath changes in this PR or first merge this PR?

coollog · 2018-09-19T03:07:50Z

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


    private Builder() {
      for (LayerType layerType : LayerType.values()) {
        layerFilesMap.put(layerType, new ArrayList<>());
+        extractionPathMap.put(layerType, "/");


Hmm, since these get...ExtractionPath methods are really only used for Docker context generation (via addIfNotEmpty) and since this default / is not actually used when the layer entries is empty, perhaps we would need to actually reorganize the API of JavaLayerConfigurations, but this can be a separate issue.

chanseokoh · 2018-09-19T17:52:02Z

Are you planning to merge the Absolute/RelativeUnixPath changes in this PR or first merge this PR?

I was about to do this. If you think you can approve this PR soon, let's merge it, and I'll work on AbsolutePath right away. Otherwise, I can work it in this PR.

coollog · 2018-09-19T17:54:31Z

Okay let's merge this first then so the changeset is smaller. I think we may possibly want to have #1009 fixed before changing this over.

chanseokoh added the PR: Not Ready label Sep 13, 2018

googlebot added the cla: yes label Sep 13, 2018

chanseokoh added 2 commits September 13, 2018 14:34

Add container.appRoot config parameter

520222a

Merge branch 'master' into i964-container.appRoot

971b95d

chanseokoh force-pushed the i964-container.appRoot branch from ddafba3 to 971b95d Compare September 13, 2018 18:36

chanseokoh mentioned this pull request Sep 13, 2018

Proposal War support with Gradle #923

Merged

loosebazooka reviewed Sep 13, 2018

View reviewed changes

chanseokoh added 18 commits September 14, 2018 10:44

Change appRoot type from String to Path

5b16ded

List digests as Set

5a7f486

Merge branch 'i985-list-cache-test-failure' into i964-container.appRoot

5024da9

Add tests

ee6fe2c

listDigests --> fetchDigests

d568f6e

Merge remote-tracking branch 'origin/i985-list-cache-test-failure' in…

886d71d

…to i964-container.appRoot

Fix typo

d86d749

Merge branch 'i985-list-cache-test-failure' into i964-container.appRoot

85c6132

Add tests

4ef4148

Add tests

a1ba423

Add tests

12face5

Update Javadocs and copyright

93b42a7

Merge remote-tracking branch 'origin/master' into i964-container.appRoot

35a64f6

Update CHANGLOG and README

c16531d

Fix test

a978e4c

Set appRoot for JavaLayerConfigurations

6330547

Check appRoot early

0e8c597

Various fixes

6489596

chanseokoh removed the PR: Not Ready label Sep 14, 2018

Add tests

713c673

chanseokoh force-pushed the i964-container.appRoot branch from 6b01f13 to 713c673 Compare September 14, 2018 22:25

chanseokoh mentioned this pull request Sep 17, 2018

UnixPath class for strictly accepting Unix-style path #992

Closed

chanseokoh added 2 commits September 17, 2018 13:30

Merge branch 'master' into i964-container.appRoot

984f51c

feedback

c70f5a3

chanseokoh requested a review from a team September 17, 2018 17:50

chanseokoh commented Sep 17, 2018

View reviewed changes

chanseokoh added the PR: Not Ready label Sep 17, 2018

chanseokoh added 4 commits September 17, 2018 17:11

Accept default extraction path along with files

3b7c510

Clean up

21bec72

format

4d5154d

Merge branch 'master' into i964-container.appRoot

becad9c

chanseokoh removed the PR: Not Ready label Sep 17, 2018

coollog reviewed Sep 18, 2018

View reviewed changes

chanseokoh added 3 commits September 18, 2018 16:44

Move field / rename methods

e699be8

Rename argument

99c7d1d

Update Javadoc

2d74e9d

coollog reviewed Sep 18, 2018

View reviewed changes

chanseokoh mentioned this pull request Sep 19, 2018

Support WARs. #431

Closed

chanseokoh added the kokoro:run label Sep 19, 2018

kokoro-team removed the kokoro:run label Sep 19, 2018

chanseokoh added 2 commits September 19, 2018 13:33

Fix typo

28ac010

Fix typo

566561f

coollog reviewed Sep 19, 2018

View reviewed changes

chanseokoh mentioned this pull request Sep 19, 2018

Reorganize JavaLayerConfigurations API about getting extraction path for Docker context generation #1010

Closed

coollog approved these changes Sep 19, 2018

View reviewed changes

chanseokoh merged commit 34c4e49 into master Sep 19, 2018

chanseokoh deleted the i964-container.appRoot branch September 19, 2018 19:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add container.appRoot config parameter #984

Add container.appRoot config parameter #984

chanseokoh commented Sep 13, 2018 •

edited

Loading

loosebazooka Sep 13, 2018 •

edited

Loading

loosebazooka Sep 13, 2018

chanseokoh Sep 13, 2018

loosebazooka Sep 13, 2018

chanseokoh Sep 13, 2018

chanseokoh Sep 14, 2018

loosebazooka Sep 14, 2018

chanseokoh Sep 14, 2018

loosebazooka Sep 13, 2018

chanseokoh Sep 13, 2018

chanseokoh Sep 14, 2018

loosebazooka commented Sep 13, 2018

chanseokoh commented Sep 14, 2018

chanseokoh Sep 17, 2018

coollog Sep 17, 2018

chanseokoh commented Sep 17, 2018

chanseokoh commented Sep 17, 2018

coollog Sep 18, 2018

coollog Sep 18, 2018

coollog Sep 18, 2018

chanseokoh Sep 18, 2018 •

edited

Loading

coollog Sep 18, 2018

chanseokoh Sep 18, 2018

coollog Sep 19, 2018

chanseokoh Sep 19, 2018

chanseokoh Sep 19, 2018

coollog left a comment

coollog Sep 19, 2018

chanseokoh commented Sep 19, 2018 •

edited

Loading

coollog commented Sep 19, 2018

Add container.appRoot config parameter #984

Add container.appRoot config parameter #984

Conversation

chanseokoh commented Sep 13, 2018 • edited Loading

loosebazooka Sep 13, 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

loosebazooka commented Sep 13, 2018

chanseokoh commented Sep 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chanseokoh commented Sep 17, 2018

chanseokoh commented Sep 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chanseokoh Sep 18, 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

coollog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chanseokoh commented Sep 19, 2018 • edited Loading

coollog commented Sep 19, 2018

chanseokoh commented Sep 13, 2018 •

edited

Loading

loosebazooka Sep 13, 2018 •

edited

Loading

chanseokoh Sep 18, 2018 •

edited

Loading

chanseokoh commented Sep 19, 2018 •

edited

Loading