Add support for SNAPSHOT dependencies layer #584
Conversation
loosebazooka
force-pushed
the
snapshot-deps
branch
3 times, most recently
from
bb71d9f
to
3fa7cda
Compare
| @@ -37,6 +37,7 @@ | |||
| ImmutableList<String> classPaths = | |||
| ImmutableList.of( | |||
| sourceFilesConfiguration.getDependenciesPathOnImage() + "*", | |||
| sourceFilesConfiguration.getSnapshotDependenciesPathOnImage() + "*", | |||
There was a problem hiding this comment.
Is there any harm to putting the snapshot dependencies in the same /app/libs/ directory?
There was a problem hiding this comment.
We also might not want to have the snapshot layer if there are no snapshot dependencies, so having that directory added to the classpath may cause directory not found exceptions?
There was a problem hiding this comment.
Yeah, I don't have any reason reason for picking a separate directory. Let me change it.
|
+1
…On Thu, Jul 12, 2018 at 12:45 PM Appu ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In
jib-core/src/main/java/com/google/cloud/tools/jib/builder/EntrypointBuilder.java
<#584 (comment)>
:
> @@ -37,6 +37,7 @@
ImmutableList<String> classPaths =
ImmutableList.of(
sourceFilesConfiguration.getDependenciesPathOnImage() + "*",
+ sourceFilesConfiguration.getSnapshotDependenciesPathOnImage() + "*",
Yeah, I don't have any reason reason for picking a separate directory. Let
me change it.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#584 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AHf5HReu1qQq0-6Exd7_9g_DQwiruVGrks5uF30sgaJpZM4VNFwH>
.
|
loosebazooka
force-pushed
the
snapshot-deps
branch
from
7f86750
to
6d165fe
Compare
| @@ -63,6 +63,17 @@ | |||
| sourceFilesConfiguration.getDependenciesPathOnImage()) | |||
| .build(), | |||
| cache)) | |||
| .add( | |||
There was a problem hiding this comment.
We should probably not add this if there are no snapshot dependencies.
| try (Stream<Path> fileStream = | ||
| Files.list(Paths.get(Resources.getResource(resourcePath).toURI()))) { | ||
| return fileStream.collect(ImmutableList.toImmutableList()); | ||
| public static Builder builder() { |
There was a problem hiding this comment.
ordering nit: for convention in the codebase, we're trying to follow the class element ordering of:
- public->private
- static->non-static
- members->constructor->methods
loosebazooka
force-pushed
the
snapshot-deps
branch
from
30782d7
to
ea02e67
Compare
| @@ -92,11 +94,16 @@ private GradleSourceFilesConfiguration(Project project, GradleBuildLogger gradle | |||
| if (resourcesOutputDirectory.equals(dependencyFile.toPath())) { | |||
| continue; | |||
| } | |||
| dependenciesFiles.add(dependencyFile.toPath()); | |||
| if (dependencyFile.getName().contains("SNAPSHOT")) { | |||
There was a problem hiding this comment.
I guess it would be better to use the Grade API for asking about whether a dependency is a SNAPSHOT or not. Don't know at the top of my head how to do that, I'm afraid.
There was a problem hiding this comment.
It does look like Gradle supports explicitly indicating that a dependency may be changing and provides an API (docs). Opened #694.
Partial solution to #403
The behavior of this change is to separate out snapshot and non-snapshot dependencies as their own layers but still writes them both to
/app/libs.So
SourceFilesConfigurationonly add a getter on location of the snapshot dependency files, but adds no extra configuration paramter for the location to write those files (it's just shared with regular dependencies)