Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Minor refactoring to prepare for extra directory targets #2436

Merged
merged 2 commits into from
Apr 30, 2020
Merged

Minor refactoring to prepare for extra directory targets #2436

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6548c75
Minor refactoring to prepare for extra directory targets
TadCordle Apr 28, 2020
48dc792
Merge branch 'master' of github.com:GoogleContainerTools/jib into 158…
TadCordle Apr 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions ...gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/GradleRawConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,14 @@

package com.google.cloud.tools.jib.gradle;

import com.google.cloud.tools.jib.api.buildplan.AbsoluteUnixPath;
import com.google.cloud.tools.jib.api.buildplan.FilePermissions;
import com.google.cloud.tools.jib.api.buildplan.ImageFormat;
import com.google.cloud.tools.jib.plugins.common.AuthProperty;
import com.google.cloud.tools.jib.plugins.common.RawConfiguration;
import java.nio.file.Path;
import java.util.Collections;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
Expand Down Expand Up @@ -157,8 +159,12 @@ public String getCreationTime() {
}

@Override
public List<Path> getExtraDirectories() {
return jibExtension.getExtraDirectories().getPaths();
public Map<Path, AbsoluteUnixPath> getExtraDirectories() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the return type may depend on how we will define the new configuration. I guess one way to translate this is

    extraDirectories {
      // not sure if you can use `/`, '\,', or `C:\` as a key name in a map.
      paths = [/path/to/extra/dir: '/target/dir'] // while maintaining backward-compatibility of string list
      permissions = [
        '/path/on/container/file1': 744,
        '/path/on/container/file2': 123
      ]
    }

Using a path as a map key probably won't work in Maven either. So I reckon you'll have it like

    extraDirectories {
      paths { // directory1
         src = ...
         target = ... 
      }
      paths { // directory2
         src = ...
         target = ... 
      }
    }

?

I think returning Map<Path, AbsoluteUnixPath> here will still work even if you define a nested structure for paths, but I wonder if it is better to define a nested class (such as RawConfiguration.ExtraDirectoriesConfiguration) if we are going to have a nested structure anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I guess that's true, this could probably be closer to how AuthProperty is setup. I guess the question I have is, is RawConfiguration actually meant to imply the layout of the maven/gradle configurations, or is it just meant to purely be a translation layer? I just felt a map was simpler; if we add a nested class in RawConfiguration, we still need to translate the maven/gradle-specific nested configuration classes into the plugins-common version, so it's the same amount of work plus one extra file. The only up-side I can immediately see is that RawConfiguration would more closely resemble the maven/gradle configuration. Is that what we want to stick with?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a map also forces people to avoid specifying 2 target directories for the same source directory. Is that something we want to support?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the question I have is, is RawConfiguration actually meant to imply the layout of the maven/gradle configurations, or is it just meant to purely be a translation layer?

From my point of view, RawConfiguration is a higher-level abstraction that doesn't necessarily have to follow the strict configuration layout. It can be independent, as long as it provides "raw config values" in a reasonable way. It's just that, because of the "raw" nature, I think in many cases it'll naturally resemble the configuration layout. The point is, we can choose whatever option that provides a better abstraction that makes sense. For example, returning a Map doesn't describe what the keys and the values mean; the caller should have advanced knowledge that the key is the source and the value is the target. In this sense, we could say returning a structured class has an advantage. But I am not so against using Map. This is up for discussions.

Using a map also forces people to avoid specifying 2 target directories for the same source directory. Is that something we want to support?

I don't think we want to support that, at least not at the moment. Anyways, I think we can enforce this constraint when using a structured class too.

Map<Path, AbsoluteUnixPath> directoryMap = new LinkedHashMap<>();
for (Path path : jibExtension.getExtraDirectories().getPaths()) {
directoryMap.put(path, AbsoluteUnixPath.get("/"));
}
return directoryMap;
}

@Override
Expand Down
10 changes: 8 additions & 2 deletions jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/MavenRawConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,13 @@

package com.google.cloud.tools.jib.maven;

import com.google.cloud.tools.jib.api.buildplan.AbsoluteUnixPath;
import com.google.cloud.tools.jib.api.buildplan.FilePermissions;
import com.google.cloud.tools.jib.api.buildplan.ImageFormat;
import com.google.cloud.tools.jib.plugins.common.AuthProperty;
import com.google.cloud.tools.jib.plugins.common.RawConfiguration;
import java.nio.file.Path;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
Expand Down Expand Up @@ -161,8 +163,12 @@ public String getCreationTime() {
}

@Override
public List<Path> getExtraDirectories() {
return MojoCommon.getExtraDirectories(jibPluginConfiguration);
public Map<Path, AbsoluteUnixPath> getExtraDirectories() {
Map<Path, AbsoluteUnixPath> directoryMap = new LinkedHashMap<>();
for (Path path : MojoCommon.getExtraDirectories(jibPluginConfiguration)) {
directoryMap.put(path, AbsoluteUnixPath.get("/"));
}
return directoryMap;
}

@Override
Expand Down
10 changes: 6 additions & 4 deletions ...n/src/main/java/com/google/cloud/tools/jib/plugins/common/JavaContainerBuilderHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,14 +42,16 @@ public class JavaContainerBuilderHelper {
/**
* Returns a {@link FileEntriesLayer} for adding the extra directory to the container.
*
* @param extraDirectory the source extra directory path
* @param sourceDirectory the source extra directory path
* @param targetDirectory the root directory on the container to place the files in
* @param extraDirectoryPermissions map from path on container to file permissions
* @param modificationTimeProvider file modification time provider
* @return a {@link FileEntriesLayer} for adding the extra directory to the container
* @throws IOException if walking the extra directory fails
*/
public static FileEntriesLayer extraDirectoryLayerConfiguration(
Path extraDirectory,
Path sourceDirectory,
AbsoluteUnixPath targetDirectory,
Map<String, FilePermissions> extraDirectoryPermissions,
BiFunction<Path, AbsoluteUnixPath, Instant> modificationTimeProvider)
throws IOException {
Expand All @@ -61,12 +63,12 @@ public static FileEntriesLayer extraDirectoryLayerConfiguration(
FileSystems.getDefault().getPathMatcher("glob:" + entry.getKey()), entry.getValue());
}

new DirectoryWalker(extraDirectory)
new DirectoryWalker(sourceDirectory)
.filterRoot()
.walk(
localPath -> {
AbsoluteUnixPath pathOnContainer =
AbsoluteUnixPath.get("/").resolve(extraDirectory.relativize(localPath));
targetDirectory.resolve(sourceDirectory.relativize(localPath));
Instant modificationTime = modificationTimeProvider.apply(localPath, pathOnContainer);
FilePermissions permissions =
extraDirectoryPermissions.get(pathOnContainer.toString());
Expand Down
11 changes: 8 additions & 3 deletions ...src/main/java/com/google/cloud/tools/jib/plugins/common/PluginConfigurationProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
import java.util.ArrayList;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.function.BiFunction;
Expand Down Expand Up @@ -396,11 +397,15 @@ static JibContainerBuilder processCommonConfiguration(
getCreationTime(rawConfiguration.getCreationTime(), projectProperties));

// Adds all the extra files.
for (Path directory : rawConfiguration.getExtraDirectories()) {
if (Files.exists(directory)) {
for (Map.Entry<Path, AbsoluteUnixPath> entry :
rawConfiguration.getExtraDirectories().entrySet()) {
Path sourceDirectory = entry.getKey();
AbsoluteUnixPath targetDirectory = entry.getValue();
if (Files.exists(sourceDirectory)) {
jibContainerBuilder.addFileEntriesLayer(
JavaContainerBuilderHelper.extraDirectoryLayerConfiguration(
directory,
sourceDirectory,
targetDirectory,
rawConfiguration.getExtraDirectoryPermissions(),
modificationTimeProvider));
}
Expand Down
3 changes: 2 additions & 1 deletion ...gins-common/src/main/java/com/google/cloud/tools/jib/plugins/common/RawConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

package com.google.cloud.tools.jib.plugins.common;

import com.google.cloud.tools.jib.api.buildplan.AbsoluteUnixPath;
import com.google.cloud.tools.jib.api.buildplan.FilePermissions;
import com.google.cloud.tools.jib.api.buildplan.ImageFormat;
import java.nio.file.Path;
Expand Down Expand Up @@ -85,7 +86,7 @@ static interface ExtensionConfiguration {

String getCreationTime();

List<Path> getExtraDirectories();
Map<Path, AbsoluteUnixPath> getExtraDirectories();

Map<String, FilePermissions> getExtraDirectoryPermissions();

Expand Down
15 changes: 12 additions & 3 deletions ...c/test/java/com/google/cloud/tools/jib/plugins/common/JavaContainerBuilderHelperTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,10 @@ public void testExtraDirectoryLayerConfiguration() throws URISyntaxException, IO
Path extraFilesDirectory = Paths.get(Resources.getResource("core/layer").toURI());
FileEntriesLayer layerConfiguration =
JavaContainerBuilderHelper.extraDirectoryLayerConfiguration(
extraFilesDirectory, Collections.emptyMap(), (ignored1, ignored2) -> Instant.EPOCH);
extraFilesDirectory,
AbsoluteUnixPath.get("/"),
Collections.emptyMap(),
(ignored1, ignored2) -> Instant.EPOCH);
assertSourcePathsUnordered(
Arrays.asList(
extraFilesDirectory.resolve("a"),
Expand All @@ -114,7 +117,10 @@ public void testExtraDirectoryLayerConfiguration_globPermissions()
FilePermissions.fromOctalString("765"));
FileEntriesLayer fileEntriesLayer =
JavaContainerBuilderHelper.extraDirectoryLayerConfiguration(
extraFilesDirectory, permissionsMap, (ignored1, ignored2) -> Instant.EPOCH);
extraFilesDirectory,
AbsoluteUnixPath.get("/"),
permissionsMap,
(ignored1, ignored2) -> Instant.EPOCH);
assertExtractionPathsUnordered(
Arrays.asList("/a", "/a/b", "/a/b/bar", "/c", "/c/cat", "/foo"),
fileEntriesLayer.getEntries());
Expand Down Expand Up @@ -150,7 +156,10 @@ public void testExtraDirectoryLayerConfiguration_overlappingPermissions()
FilePermissions.fromOctalString("765"));
FileEntriesLayer fileEntriesLayer =
JavaContainerBuilderHelper.extraDirectoryLayerConfiguration(
extraFilesDirectory, permissionsMap, (ignored1, ignored2) -> Instant.EPOCH);
extraFilesDirectory,
AbsoluteUnixPath.get("/"),
permissionsMap,
(ignored1, ignored2) -> Instant.EPOCH);
assertExtractionPathsUnordered(
Arrays.asList("/a", "/a/b", "/a/b/bar", "/c", "/c/cat", "/foo"),
fileEntriesLayer.getEntries());
Expand Down
19 changes: 13 additions & 6 deletions ...test/java/com/google/cloud/tools/jib/plugins/common/PluginConfigurationProcessorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,8 +111,6 @@ public void setUp() throws IOException, InvalidImageReferenceException, Inferred
Mockito.when(rawConfiguration.getAppRoot()).thenReturn("/app");
Mockito.when(rawConfiguration.getFilesModificationTime()).thenReturn("EPOCH_PLUS_SECOND");
Mockito.when(rawConfiguration.getCreationTime()).thenReturn("EPOCH");
Mockito.when(rawConfiguration.getExtraDirectories())
.thenReturn(Arrays.asList(Paths.get("nonexistent/path")));
Mockito.when(rawConfiguration.getContainerizingMode()).thenReturn("exploded");
Mockito.when(projectProperties.getToolName()).thenReturn("tool");
Mockito.when(projectProperties.getToolVersion()).thenReturn("tool-version");
Expand Down Expand Up @@ -158,9 +156,10 @@ public void testPluginConfigurationProcessor_extraDirectory()
InvalidContainerizingModeException, InvalidFilesModificationTimeException,
InvalidCreationTimeException {
Path extraDirectory = Paths.get(Resources.getResource("core/layer").toURI());
Mockito.when(rawConfiguration.getExtraDirectories()).thenReturn(Arrays.asList(extraDirectory));
Mockito.when(rawConfiguration.getExtraDirectories())
.thenReturn(ImmutableMap.of(extraDirectory, AbsoluteUnixPath.get("/target/dir")));
Mockito.when(rawConfiguration.getExtraDirectoryPermissions())
.thenReturn(ImmutableMap.of("/foo", FilePermissions.fromOctalString("123")));
.thenReturn(ImmutableMap.of("/target/dir/foo", FilePermissions.fromOctalString("123")));

BuildContext buildContext = getBuildContext(processCommonConfiguration());
List<FileEntry> extraFiles =
Expand All @@ -182,13 +181,21 @@ public void testPluginConfigurationProcessor_extraDirectory()
extraDirectory.resolve("foo")),
extraFiles);
assertExtractionPathsUnordered(
Arrays.asList("/a", "/a/b", "/a/b/bar", "/c", "/c/cat", "/foo"), extraFiles);
Arrays.asList(
"/target/dir/a",
"/target/dir/a/b",
"/target/dir/a/b/bar",
"/target/dir/c",
"/target/dir/c/cat",
"/target/dir/foo"),
extraFiles);

Optional<FileEntry> fooEntry =
extraFiles
.stream()
.filter(
layerEntry -> layerEntry.getExtractionPath().equals(AbsoluteUnixPath.get("/foo")))
layerEntry ->
layerEntry.getExtractionPath().equals(AbsoluteUnixPath.get("/target/dir/foo")))
.findFirst();
Assert.assertTrue(fooEntry.isPresent());
Assert.assertEquals("123", fooEntry.get().getPermissions().toOctalString());
Expand Down