-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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.
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
Add container.appRoot config parameter #984
Conversation
ddafba3
to
971b95d
Compare
@@ -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"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like the example should just use the current default value? #976
@@ -131,6 +131,11 @@ private static String mapToDockerfileString(Map<String, String> map, String comm | |||
return joiner.toString(); | |||
} | |||
|
|||
@VisibleForTesting | |||
static String getUnixPath(Path path) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think another PR has a similar utility, might be worth using the same for both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Certainly. Since that utility is not yet available, I'll add a TODO here to use the utility.
} | ||
|
||
/** 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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like addEntryRecursively actually boils down to some Unix path conversion eventually.
public String getAbsoluteExtractionPathString() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems like we might need to use a custom validator that is not OS specific.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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");
oops sorry I jumped the gun on the review there a little. Didn't see the |
…to i964-container.appRoot
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. |
6b01f13
to
713c673
Compare
@@ -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( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea that sounds good. One nit is to have it be setClassFiles(List<Path> classFiles, String extractionPath)
to be more like LayerEntry
.
Will re-design this per #984 (comment). Not ready for review until then. |
Ready for review |
jib-core/src/main/java/com/google/cloud/tools/jib/configuration/ContainerConfiguration.java
Outdated
Show resolved
Hide resolved
@@ -134,10 +146,13 @@ public static Builder builder() { | |||
} | |||
|
|||
private final ImmutableMap<LayerType, LayerConfiguration> layerConfigurationMap; | |||
private final ImmutableMap<LayerType, String> defaultExtractionPathMap; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just extractionPathMap
? This doesn't look like it's getting any defaults.
this.layerConfigurationMap = layerConfigurationsMap; | ||
ImmutableMap<LayerType, LayerConfiguration> layerConfigurationsMap, | ||
ImmutableMap<LayerType, String> defaultExtractionPathMap) { | ||
layerConfigurationMap = layerConfigurationsMap; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we should name these both layerConfigurationsMap
so this can be the more familiar this.x = x
form?
@@ -170,7 +185,35 @@ private JavaLayerConfigurations( | |||
return getLayerEntries(LayerType.EXPLODED_WAR); | |||
} | |||
|
|||
public String getDependencyDefaultExtractionPath() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these getting defaults or the actual extraction path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll drop Default
from the name.
jib-core/src/main/java/com/google/cloud/tools/jib/frontend/JavaLayerConfigurations.java
Show resolved
Hide resolved
|
||
private Builder() { | ||
for (LayerType layerType : LayerType.values()) { | ||
layerFilesMap.put(layerType, new ArrayList<>()); | ||
extractionPathMap.put(layerType, "/"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's good to have "/". Otherwise, we may go into the business of returning Optional<String>
for getResourceExtractionPath()
instead of String
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good idea. I was thinking the same. I'll file an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filed #1010.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you planning to merge the Absolute/RelativeUnixPath
changes in this PR or first merge this PR?
|
||
private Builder() { | ||
for (LayerType layerType : LayerType.values()) { | ||
layerFilesMap.put(layerType, new ArrayList<>()); | ||
extractionPathMap.put(layerType, "/"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
I was about to do this. If you think you can approve this PR soon, let's merge it, and I'll work on |
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. |
Fixes #964