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.

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

Removes SourceFilesConfiguration in favor of list of LayerConfiguration in BuildConfiguration. #516

Merged
merged 27 commits into from
Jul 25, 2018
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
b468528
Removes SourceFilesConfiguration in favor of list of LayerConfigurati…
coollog Jul 6, 2018
69e6b57
Applies to jib-gradle-plugin.
coollog Jul 6, 2018
84f5ef6
Fixes format.
coollog Jul 10, 2018
fd6daaf
Merge branch 'master' into no-sourcefilesconfiguration
coollog Jul 19, 2018
780332a
Merge branch 'master' into no-sourcefilesconfiguration
coollog Jul 19, 2018
e3d835d
Fixes auth issue.
coollog Jul 19, 2018
fd46ee2
Merge branch 'master' into no-sourcefilesconfiguration
coollog Jul 19, 2018
4736f4d
Merge branch 'master' into no-sourcefilesconfiguration
coollog Jul 20, 2018
00176fe
Refactors entrypoint building.
coollog Jul 20, 2018
36791ac
Fixes tests.
coollog Jul 20, 2018
fcde23c
Fixes jib-gradle-plugin.
coollog Jul 20, 2018
06e5b46
Changes for jib-maven-plugin.
coollog Jul 20, 2018
0bc2536
Merge branch 'master' into no-sourcefilesconfiguration
coollog Jul 20, 2018
7162e85
Simplifies DockerContextGenerator and labels layers to avoid indexing.
coollog Jul 21, 2018
7c28ffe
Merge branch 'master' into no-sourcefilesconfiguration
coollog Jul 21, 2018
e871c14
Fixes words.
coollog Jul 21, 2018
77339ff
Prints all layer entries.
coollog Jul 21, 2018
614dbc7
Fixes format.
coollog Jul 22, 2018
5a8247a
Changes fields to nullable.
coollog Jul 24, 2018
bc46b0b
Renames JavaEntrypointConstructor.
coollog Jul 24, 2018
72abd98
Merge branch 'master' into no-sourcefilesconfiguration
coollog Jul 24, 2018
0ee1e86
Some more comments.
coollog Jul 24, 2018
3596206
Fixes format.
coollog Jul 24, 2018
d7da59d
Fixes CHANGLOG.
coollog Jul 24, 2018
69764a0
A typo.
coollog Jul 24, 2018
19db093
Fixes format.
coollog Jul 25, 2018
bbe18c4
Merge branch 'master' into no-sourcefilesconfiguration
coollog Jul 25, 2018
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
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,23 @@
import com.google.cloud.tools.jib.cache.CacheDirectoryNotOwnedException;
import com.google.cloud.tools.jib.cache.CacheMetadataCorruptedException;
import com.google.cloud.tools.jib.cache.Caches;
import com.google.cloud.tools.jib.configuration.LayerConfiguration;
import com.google.cloud.tools.jib.frontend.ExposedPortsParser;
import com.google.cloud.tools.jib.frontend.JavaEntrypointBuilder;
import com.google.cloud.tools.jib.image.ImageReference;
import com.google.cloud.tools.jib.image.InvalidImageReferenceException;
import com.google.cloud.tools.jib.registry.LocalRegistry;
import com.google.common.collect.ImmutableList;
import com.google.common.io.Resources;
import java.io.IOException;
import java.net.URISyntaxException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.Arrays;
import java.util.Collections;
import java.util.concurrent.ExecutionException;
import java.util.stream.Stream;
import org.hamcrest.CoreMatchers;
import org.junit.Assert;
import org.junit.Before;
Expand All @@ -42,25 +49,36 @@
/** Integration tests for {@link BuildSteps}. */
public class BuildStepsIntegrationTest {

/** Lists the files in the {@code resourcePath} resources directory. */
private static ImmutableList<Path> getFilesList(String resourcePath)
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is specifically for resources maybe it should be renamed to getResourceFilesList.

throws URISyntaxException, IOException {
try (Stream<Path> fileStream =
Files.list(Paths.get(Resources.getResource(resourcePath).toURI()))) {
return fileStream.collect(ImmutableList.toImmutableList());
}
}

@ClassRule public static LocalRegistry localRegistry = new LocalRegistry(5000);

private static final TestBuildLogger logger = new TestBuildLogger();

@Rule public TemporaryFolder temporaryCacheDirectory = new TemporaryFolder();

@Rule public TemporaryFolder temporaryTarOutput = new TemporaryFolder();
@Rule public TemporaryFolder temporaryFolder = new TemporaryFolder();
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 it's harmless to have final. Is the repo generally lenient about final?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should definitely have final where-ever applicable and have things be non-final unless absolutely necessary.


private SourceFilesConfiguration sourceFilesConfiguration;
private ImmutableList<LayerConfiguration> fakeLayerConfigurations;

@Before
public void setUp() throws IOException, URISyntaxException {
sourceFilesConfiguration =
TestSourceFilesConfiguration.builder()
.withClasses()
.withDependencies()
.withSnapshotDependencies()
.withResources()
.build();
fakeLayerConfigurations =
ImmutableList.of(
LayerConfiguration.builder()
.addEntry(getFilesList("application/dependencies"), "/app/libs/")
.build(),
LayerConfiguration.builder()
.addEntry(getFilesList("application/resources"), "/app/resources/")
.build(),
LayerConfiguration.builder()
.addEntry(getFilesList("application/classes"), "/app/classes/")
.build());
}

@Test
Expand All @@ -72,12 +90,16 @@ public void testSteps_forBuildToDockerRegistry()
BuildConfiguration.builder(logger)
.setBaseImage(ImageReference.of("gcr.io", "distroless/java", "latest"))
.setTargetImage(ImageReference.of("localhost:5000", "testimage", "testtag"))
.setMainClass("HelloWorld")
.setJavaArguments(Collections.singletonList("An argument."))
.setExposedPorts(
ExposedPortsParser.parse(Arrays.asList("1000", "2000-2002/tcp", "3000/udp")))
.setAllowHttp(true)
.setLayerConfigurations(fakeLayerConfigurations)
.setEntrypoint(
JavaEntrypointBuilder.makeDefaultEntrypoint(
Collections.emptyList(), "HelloWorld"))
.build());
buildImageSteps.run();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we running this 3 times now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, removing.


long lastTime = System.nanoTime();
buildImageSteps.run();
Expand Down Expand Up @@ -110,9 +132,12 @@ public void testSteps_forBuildToDockerRegistry_dockerHubBaseImage()
BuildConfiguration.builder(logger)
.setBaseImage(ImageReference.parse("openjdk:8-jre-alpine"))
.setTargetImage(ImageReference.of("localhost:5000", "testimage", "testtag"))
.setMainClass("HelloWorld")
.setJavaArguments(Collections.singletonList("An argument."))
.setAllowHttp(true)
.setLayerConfigurations(fakeLayerConfigurations)
.setEntrypoint(
JavaEntrypointBuilder.makeDefaultEntrypoint(
Collections.emptyList(), "HelloWorld"))
.build())
.run();

Expand All @@ -130,16 +155,17 @@ public void testSteps_forBuildToDockerDaemon()
BuildConfiguration.builder(logger)
.setBaseImage(ImageReference.of("gcr.io", "distroless/java", "latest"))
.setTargetImage(ImageReference.of(null, "testdocker", null))
.setMainClass("HelloWorld")
.setJavaArguments(Collections.singletonList("An argument."))
.setExposedPorts(
ExposedPortsParser.parse(Arrays.asList("1000", "2000-2002/tcp", "3000/udp")))
.setLayerConfigurations(fakeLayerConfigurations)
.setEntrypoint(
JavaEntrypointBuilder.makeDefaultEntrypoint(Collections.emptyList(), "HelloWorld"))
.build();

Path cacheDirectory = temporaryCacheDirectory.newFolder().toPath();
Path cacheDirectory = temporaryFolder.newFolder().toPath();
BuildSteps.forBuildToDockerDaemon(
buildConfiguration,
sourceFilesConfiguration,
new Caches.Initializer(cacheDirectory, false, cacheDirectory, false))
.run();

Expand All @@ -164,16 +190,17 @@ public void testSteps_forBuildToTarball()
BuildConfiguration.builder(logger)
.setBaseImage(ImageReference.of("gcr.io", "distroless/java", "latest"))
.setTargetImage(ImageReference.of(null, "testtar", null))
.setMainClass("HelloWorld")
.setJavaArguments(Collections.singletonList("An argument."))
.setLayerConfigurations(fakeLayerConfigurations)
.setEntrypoint(
JavaEntrypointBuilder.makeDefaultEntrypoint(Collections.emptyList(), "HelloWorld"))
.build();

Path outputPath = temporaryTarOutput.newFolder().toPath().resolve("test.tar");
Path cacheDirectory = temporaryCacheDirectory.newFolder().toPath();
Path outputPath = temporaryFolder.newFolder().toPath().resolve("test.tar");
Path cacheDirectory = temporaryFolder.newFolder().toPath();
BuildSteps.forBuildToTar(
outputPath,
buildConfiguration,
sourceFilesConfiguration,
new Caches.Initializer(cacheDirectory, false, cacheDirectory, false))
.run();

Expand All @@ -183,10 +210,8 @@ public void testSteps_forBuildToTarball()
}

private BuildSteps getBuildSteps(BuildConfiguration buildConfiguration) throws IOException {
Path cacheDirectory = temporaryCacheDirectory.newFolder().toPath();
Path cacheDirectory = temporaryFolder.newFolder().toPath();
return BuildSteps.forBuildToDockerRegistry(
buildConfiguration,
sourceFilesConfiguration,
new Caches.Initializer(cacheDirectory, false, cacheDirectory, false));
buildConfiguration, new Caches.Initializer(cacheDirectory, false, cacheDirectory, false));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -49,16 +49,15 @@ public static class Builder {
@Nullable private ImageReference targetImageReference;
@Nullable private String targetImageCredentialHelperName;
@Nullable private RegistryCredentials knownTargetRegistryCredentials;
@Nullable private String mainClass;
private ImmutableList<String> javaArguments = ImmutableList.of();
private ImmutableList<String> jvmFlags = ImmutableList.of();
private ImmutableMap<String, String> environmentMap = ImmutableMap.of();
private ImmutableList<Port> exposedPorts = ImmutableList.of();
private Class<? extends BuildableManifestTemplate> targetFormat = V22ManifestTemplate.class;
@Nullable private CacheConfiguration applicationLayersCacheConfiguration;
@Nullable private CacheConfiguration baseImageLayersCacheConfiguration;
private boolean allowHttp = false;
@Nullable private LayerConfiguration extraFilesLayerConfiguration;
private ImmutableList<LayerConfiguration> layerConfigurations = ImmutableList.of();
private ImmutableList<String> entrypoint = ImmutableList.of();
Copy link
Contributor

Choose a reason for hiding this comment

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

This is very much a nit, so feel free to ignore, but for organization it'd be nice to put entrypoint next to javaArguments instead of at the end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good

Copy link
Contributor

Choose a reason for hiding this comment

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

(This same ordering happens in a bunch of other places too, but I didn't want to overload the comments.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, maybe we can leave that to another PR that can set up a nice ordering pattern for all of these parameters.


private BuildLogger buildLogger;

Expand Down Expand Up @@ -98,11 +97,6 @@ public Builder setKnownTargetRegistryCredentials(
return this;
}

public Builder setMainClass(@Nullable String mainClass) {
this.mainClass = mainClass;
return this;
}

public Builder setJavaArguments(@Nullable List<String> javaArguments) {
if (javaArguments != null) {
Preconditions.checkArgument(!javaArguments.contains(null));
Expand All @@ -111,14 +105,6 @@ public Builder setJavaArguments(@Nullable List<String> javaArguments) {
return this;
}

public Builder setJvmFlags(@Nullable List<String> jvmFlags) {
if (jvmFlags != null) {
Preconditions.checkArgument(!jvmFlags.contains(null));
this.jvmFlags = ImmutableList.copyOf(jvmFlags);
}
return this;
}

public Builder setEnvironment(@Nullable Map<String, String> environmentMap) {
if (environmentMap != null) {
Preconditions.checkArgument(
Expand Down Expand Up @@ -178,14 +164,13 @@ public Builder setAllowHttp(boolean allowHttp) {
}

/**
* Sets the {@link LayerConfiguration} for an extra layer.
* Sets the layers to build.
*
* @param extraFilesLayerConfiguration the layer configuration for the extra layer
* @param layerConfigurations the configurations for the layers
* @return this
*/
public Builder setExtraFilesLayerConfiguration(
@Nullable LayerConfiguration extraFilesLayerConfiguration) {
this.extraFilesLayerConfiguration = extraFilesLayerConfiguration;
public Builder setLayerConfigurations(List<LayerConfiguration> layerConfigurations) {
this.layerConfigurations = ImmutableList.copyOf(layerConfigurations);
return this;
}

Expand All @@ -200,6 +185,20 @@ public Builder setCreationTime(Instant creationTime) {
return this;
}

/**
* Sets the container entrypoint.
*
* @param entrypoint the tokenized command to run when the container starts
* @return this
*/
public Builder setEntrypoint(@Nullable List<String> entrypoint) {
Copy link
Member

Choose a reason for hiding this comment

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

What is the point of allowing null? Also, I think we should document the behavior: "passing null does nothing; it does not clear the previous value."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The point of allowing null is to allow for building an image without an entrypoint (since it's an optional field on the image format). This can also be useful for when we propagate base image container configuration so fields like entrypoint could be propagated. But yes, we should defintely document the behavior.

Copy link
Member

Choose a reason for hiding this comment

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

If so, why not just clear the entrypoint if null is given? That's more intuitive and what I'd expect normally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes a lot of sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or, actually - I think we should probably have the semantics be like:

null - if base image has entrypoint, propagate; otherwise, don't set an entrypoint
non-null - set entrypoint to what is given (empty would be empty entrypoint, etc.)

if (entrypoint != null) {
Preconditions.checkArgument(!entrypoint.contains(null));
this.entrypoint = ImmutableList.copyOf(entrypoint);
}
return this;
}

/** @return the corresponding build configuration */
public BuildConfiguration build() {
// Validates the parameters.
Expand All @@ -210,13 +209,10 @@ public BuildConfiguration build() {
if (targetImageReference == null) {
errorMessages.add("target image is required but not set");
}
if (mainClass == null) {
errorMessages.add("main class is required but not set");
}

switch (errorMessages.size()) {
case 0: // No errors
if (baseImageReference == null || targetImageReference == null || mainClass == null) {
if (baseImageReference == null || targetImageReference == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we error if entrypoint is empty like we used to error when mainClass was null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm intending it to be possibly be empty to allow for propagation of entrypoint from base images in the future (when this is used as a library).

throw new IllegalStateException("Required fields should not be null");
}
if (baseImageReference.usesDefaultTag()) {
Expand All @@ -234,16 +230,15 @@ public BuildConfiguration build() {
targetImageReference,
targetImageCredentialHelperName,
knownTargetRegistryCredentials,
mainClass,
javaArguments,
jvmFlags,
environmentMap,
exposedPorts,
targetFormat,
applicationLayersCacheConfiguration,
baseImageLayersCacheConfiguration,
allowHttp,
extraFilesLayerConfiguration);
layerConfigurations,
entrypoint);

case 1:
throw new IllegalStateException(errorMessages.get(0));
Expand Down Expand Up @@ -294,16 +289,15 @@ public static Builder builder(BuildLogger buildLogger) {
private final ImageReference targetImageReference;
@Nullable private final String targetImageCredentialHelperName;
@Nullable private final RegistryCredentials knownTargetRegistryCredentials;
private final String mainClass;
private final ImmutableList<String> javaArguments;
private final ImmutableList<String> jvmFlags;
private final ImmutableMap<String, String> environmentMap;
private final ImmutableList<Port> exposedPorts;
private final Class<? extends BuildableManifestTemplate> targetFormat;
@Nullable private final CacheConfiguration applicationLayersCacheConfiguration;
@Nullable private final CacheConfiguration baseImageLayersCacheConfiguration;
private final boolean allowHttp;
@Nullable private final LayerConfiguration extraFilesLayerConfiguration;
private final ImmutableList<LayerConfiguration> layerConfigurations;
private final ImmutableList<String> entrypoint;

/** Instantiate with {@link Builder#build}. */
private BuildConfiguration(
Expand All @@ -315,16 +309,15 @@ private BuildConfiguration(
ImageReference targetImageReference,
@Nullable String targetImageCredentialHelperName,
@Nullable RegistryCredentials knownTargetRegistryCredentials,
String mainClass,
ImmutableList<String> javaArguments,
ImmutableList<String> jvmFlags,
ImmutableMap<String, String> environmentMap,
ImmutableList<Port> exposedPorts,
Class<? extends BuildableManifestTemplate> targetFormat,
@Nullable CacheConfiguration applicationLayersCacheConfiguration,
@Nullable CacheConfiguration baseImageLayersCacheConfiguration,
boolean allowHttp,
@Nullable LayerConfiguration extraFilesLayerConfiguration) {
ImmutableList<LayerConfiguration> layerConfigurations,
ImmutableList<String> entrypoint) {
this.buildLogger = buildLogger;
this.creationTime = creationTime;
this.baseImageReference = baseImageReference;
Expand All @@ -333,16 +326,15 @@ private BuildConfiguration(
this.targetImageReference = targetImageReference;
this.targetImageCredentialHelperName = targetImageCredentialHelperName;
this.knownTargetRegistryCredentials = knownTargetRegistryCredentials;
this.mainClass = mainClass;
this.javaArguments = javaArguments;
this.jvmFlags = jvmFlags;
this.environmentMap = environmentMap;
this.exposedPorts = exposedPorts;
this.targetFormat = targetFormat;
this.applicationLayersCacheConfiguration = applicationLayersCacheConfiguration;
this.baseImageLayersCacheConfiguration = baseImageLayersCacheConfiguration;
this.allowHttp = allowHttp;
this.extraFilesLayerConfiguration = extraFilesLayerConfiguration;
this.layerConfigurations = layerConfigurations;
this.entrypoint = entrypoint;
}

public BuildLogger getBuildLogger() {
Expand Down Expand Up @@ -405,18 +397,10 @@ public RegistryCredentials getKnownTargetRegistryCredentials() {
return knownTargetRegistryCredentials;
}

public String getMainClass() {
return mainClass;
}

public ImmutableList<String> getJavaArguments() {
return javaArguments;
}

public ImmutableList<String> getJvmFlags() {
return jvmFlags;
}

public ImmutableMap<String, String> getEnvironment() {
return environmentMap;
}
Expand Down Expand Up @@ -459,12 +443,20 @@ public boolean getAllowHttp() {
}

/**
* Gets the {@link LayerConfiguration} for an extra layer.
* Gets the configurations for building the layers.
*
* @return the layer configuration
* @return the list of layer configurations
*/
@Nullable
public LayerConfiguration getExtraFilesLayerConfiguration() {
return extraFilesLayerConfiguration;
public ImmutableList<LayerConfiguration> getLayerConfigurations() {
return layerConfigurations;
}

/**
* Gets the container entrypoint.
*
* @return the list of entrypoint tokens
*/
public ImmutableList<String> getEntrypoint() {
return entrypoint;
}
}
Loading