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

Write history as part of layer metadata #877

Merged
merged 19 commits into from
Aug 28, 2018
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
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 @@ -113,6 +113,9 @@ public void testSteps_forBuildToDockerRegistry()
+ " \"2001/tcp\": {},\n"
+ " \"2002/tcp\": {},\n"
+ " \"3000/udp\": {}"));
String history = new Command("docker", "history", imageReference).run();
Assert.assertThat(history, CoreMatchers.containsString("jib"));
Assert.assertThat(history, CoreMatchers.containsString("bazel build ..."));
Assert.assertEquals(
"Hello, world. An argument.\n", new Command("docker", "run", imageReference).run());
}
Expand All @@ -138,17 +141,18 @@ public void testSteps_forBuildToDockerRegistry_dockerHubBaseImage()
public void testSteps_forBuildToDockerDaemon()
throws IOException, InterruptedException, CacheMetadataCorruptedException, ExecutionException,
CacheDirectoryNotOwnedException, CacheDirectoryCreationException {
String imageReference = "testdocker";
BuildConfiguration buildConfiguration =
getBuildConfiguration(
ImageReference.of("gcr.io", "distroless/java", "latest"),
ImageReference.of(null, "testdocker", null));
ImageReference.of(null, imageReference, null));
Path cacheDirectory = temporaryFolder.newFolder().toPath();
BuildSteps.forBuildToDockerDaemon(
buildConfiguration,
new Caches.Initializer(cacheDirectory, false, cacheDirectory, false))
.run();

String dockerContainerConfig = new Command("docker", "inspect", "testdocker").run();
String dockerContainerConfig = new Command("docker", "inspect", imageReference).run();
Assert.assertThat(
dockerContainerConfig,
CoreMatchers.containsString(
Expand All @@ -165,8 +169,11 @@ public void testSteps_forBuildToDockerDaemon()
+ " \"key1\": \"value1\",\n"
+ " \"key2\": \"value2\"\n"
+ " }"));
String history = new Command("docker", "history", imageReference).run();
Assert.assertThat(history, CoreMatchers.containsString("jib"));
Assert.assertThat(history, CoreMatchers.containsString("bazel build ..."));
Assert.assertEquals(
"Hello, world. An argument.\n", new Command("docker", "run", "testdocker").run());
"Hello, world. An argument.\n", new Command("docker", "run", imageReference).run());
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,12 @@
import com.google.cloud.tools.jib.image.Image;
import com.google.cloud.tools.jib.image.Layer;
import com.google.cloud.tools.jib.image.LayerPropertyNotFoundException;
import com.google.cloud.tools.jib.image.json.HistoryEntry;
import com.google.common.collect.ImmutableList;
import com.google.common.util.concurrent.Futures;
import com.google.common.util.concurrent.ListenableFuture;
import com.google.common.util.concurrent.ListeningExecutorService;
import java.time.Instant;
import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.Callable;
Expand Down Expand Up @@ -94,22 +96,44 @@ private Image<CachedLayer> afterCachedLayersSteps()
try (Timer ignored = new Timer(buildConfiguration.getBuildLogger(), DESCRIPTION)) {
// Constructs the image.
Image.Builder<CachedLayer> imageBuilder = Image.builder();
for (PullAndCacheBaseImageLayerStep pullAndCacheBaseImageLayerStep :
NonBlockingSteps.get(pullAndCacheBaseImageLayersStep)) {
Image<Layer> baseImage = NonBlockingSteps.get(pullBaseImageStep).getBaseImage();
ContainerConfiguration containerConfiguration =
buildConfiguration.getContainerConfiguration();

// Base image layers
List<PullAndCacheBaseImageLayerStep> baseImageLayers =
NonBlockingSteps.get(pullAndCacheBaseImageLayersStep);
for (PullAndCacheBaseImageLayerStep pullAndCacheBaseImageLayerStep : baseImageLayers) {
imageBuilder.addLayer(NonBlockingSteps.get(pullAndCacheBaseImageLayerStep));
}
for (BuildAndCacheApplicationLayerStep buildAndCacheApplicationLayerStep :
buildAndCacheApplicationLayerSteps) {
imageBuilder.addLayer(NonBlockingSteps.get(buildAndCacheApplicationLayerStep));
}

// Parameters that we passthrough from the base image
Image<Layer> baseImage = NonBlockingSteps.get(pullBaseImageStep).getBaseImage();
// Passthrough config and count non-empty history entries
int nonEmptyLayerCount = 0;
for (HistoryEntry historyObject : baseImage.getHistory()) {
imageBuilder.addHistory(historyObject);
if (!historyObject.hasLayer()) {
nonEmptyLayerCount++;
}
}
imageBuilder.addEnvironment(baseImage.getEnvironment());
imageBuilder.addLabels(baseImage.getLabels());

ContainerConfiguration containerConfiguration =
buildConfiguration.getContainerConfiguration();
// Add history elements for non-empty layers that don't have one yet
String layerCreationTime =
containerConfiguration == null
? Instant.EPOCH.toString()
: containerConfiguration.getCreationTime().toString();
for (int count = 0; count < baseImageLayers.size() - nonEmptyLayerCount; count++) {
imageBuilder.addHistory(
new HistoryEntry(layerCreationTime, null, null, "auto-generated by Jib", null));
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably want to go with a builder pattern here since it's unclear which field is which.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, "auto-generated by Jib" might not be the text we want for created_by, since created_by refers to what created the layer the history entry refers to (and that layer was not "auto-generated by Jib") - I think "unknown" would be more suitable here.

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's comment, not created_by.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah right, then my first comment applies haha.

}

// Add built layers/configuration
for (BuildAndCacheApplicationLayerStep buildAndCacheApplicationLayerStep :
buildAndCacheApplicationLayerSteps) {
imageBuilder.addLayer(NonBlockingSteps.get(buildAndCacheApplicationLayerStep));
imageBuilder.addHistory(new HistoryEntry(layerCreationTime, "Jib", "jib", null, null));
}
if (containerConfiguration != null) {
imageBuilder.addEnvironment(containerConfiguration.getEnvironmentMap());
imageBuilder.setCreated(containerConfiguration.getCreationTime());
Expand Down
23 changes: 23 additions & 0 deletions jib-core/src/main/java/com/google/cloud/tools/jib/image/Image.java
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package com.google.cloud.tools.jib.image;

import com.google.cloud.tools.jib.configuration.Port;
import com.google.cloud.tools.jib.image.json.HistoryEntry;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import java.time.Instant;
Expand All @@ -31,6 +32,7 @@ public class Image<T extends Layer> {
public static class Builder<T extends Layer> {

private final ImageLayers.Builder<T> imageLayersBuilder = ImageLayers.builder();
private ImmutableList.Builder<HistoryEntry> historyBuilder = ImmutableList.builder();
private ImmutableMap.Builder<String, String> environmentBuilder = ImmutableMap.builder();
private ImmutableMap.Builder<String, String> labelsBuilder = ImmutableMap.builder();

Expand Down Expand Up @@ -145,10 +147,22 @@ public Builder<T> addLayer(T layer) throws LayerPropertyNotFoundException {
return this;
}

/**
* Adds a history element to the image.
*
* @param history the history object to add
* @return this
*/
public Builder<T> addHistory(HistoryEntry history) {
historyBuilder.add(history);
return this;
}

public Image<T> build() {
return new Image<>(
created,
imageLayersBuilder.build(),
historyBuilder.build(),
environmentBuilder.build(),
entrypoint,
javaArguments,
Expand All @@ -167,6 +181,9 @@ public static <T extends Layer> Builder<T> builder() {
/** The layers of the image, in the order in which they are applied. */
private final ImageLayers<T> layers;

/** The commands used to build each layer of the image */
private final ImmutableList<HistoryEntry> history;

/** Environment variable definitions for running the image, in the format {@code NAME=VALUE}. */
@Nullable private final ImmutableMap<String, String> environment;

Expand All @@ -185,13 +202,15 @@ public static <T extends Layer> Builder<T> builder() {
private Image(
@Nullable Instant created,
ImageLayers<T> layers,
ImmutableList<HistoryEntry> history,
@Nullable ImmutableMap<String, String> environment,
@Nullable ImmutableList<String> entrypoint,
@Nullable ImmutableList<String> javaArguments,
@Nullable ImmutableList<Port> exposedPorts,
@Nullable ImmutableMap<String, String> labels) {
this.created = created;
this.layers = layers;
this.history = history;
this.environment = environment;
this.entrypoint = entrypoint;
this.javaArguments = javaArguments;
Expand Down Expand Up @@ -232,4 +251,8 @@ public ImmutableMap<String, String> getLabels() {
public ImmutableList<T> getLayers() {
return layers.getLayers();
}

public ImmutableList<HistoryEntry> getHistory() {
return history;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,18 @@
* "ExposedPorts": { "6000/tcp":{}, "8000/tcp":{}, "9000/tcp":{} }
* "Labels": { "com.example.label": "value" }
* },
* "history": [
* {
* "author": "Jib",
* "created": "1970-01-01T00:00:00Z",
* "created_by": "jib"
* },
* {
* "author": "Jib",
* "created": "1970-01-01T00:00:00Z",
* "created_by": "jib"
* }
* ]
* "rootfs": {
* "diff_ids": [
* "sha256:2aebd096e0e237b447781353379722157e6c2d434b9ec5a0d63f2a6f07cf90c2",
Expand Down Expand Up @@ -70,6 +82,9 @@ public class ContainerConfigurationTemplate implements JsonTemplate {
/** Execution parameters that should be used as a base when running the container. */
private final ConfigurationObjectTemplate config = new ConfigurationObjectTemplate();

/** Build commands used to create the image. */
Copy link
Contributor

Choose a reason for hiding this comment

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

"Describes the history of each layer"?

private final List<HistoryEntry> history = new ArrayList<>();

/** Layer content digests that are used to build the container filesystem. */
private final RootFilesystemObjectTemplate rootfs = new RootFilesystemObjectTemplate();

Expand Down Expand Up @@ -137,10 +152,18 @@ public void addLayerDiffId(DescriptorDigest diffId) {
rootfs.diff_ids.add(diffId);
}

public void addHistory(HistoryEntry historyEntry) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: since history is a plural list of HistoryEntrys, perhaps this method name should be addToHistory instead

history.add(historyEntry);
}

List<DescriptorDigest> getDiffIds() {
return rootfs.diff_ids;
}

List<HistoryEntry> getHistory() {
return history;
}

@Nullable
String getCreated() {
return created;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
/*
* Copyright 2018 Google LLC. All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not
* use this file except in compliance with the License. You may obtain a copy of
* the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations under
* the License.
*/

package com.google.cloud.tools.jib.image.json;

import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.google.cloud.tools.jib.json.JsonTemplate;
import java.util.Objects;
import javax.annotation.Nullable;

/**
* Represents an item in the container configuration's {@code history} list.
*
* @see <a href=https://github.com/opencontainers/image-spec/blob/master/config.md#properties>OCI
* image spec ({@code history} field)</a>
*/
@JsonIgnoreProperties(ignoreUnknown = true)
public class HistoryEntry implements JsonTemplate {

/** The RFC 3339 formatted timestamp at which the image was created. */
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: specifically section 5.6 of RFC 3339

@JsonProperty("created")
@Nullable
private String creationTimestamp;

/** The name of the author specified when committing the image. */
@JsonProperty("author")
@Nullable
private String author;

/** The command used to build the image. */
Copy link
Contributor

Choose a reason for hiding this comment

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

s/image/layer/

@JsonProperty("created_by")
@Nullable
private String createdBy;

/** A custom message set when creating the layer. */
@JsonProperty("comment")
@Nullable
private String comment;

/**
* Whether or not the entry corresponds to an empty layer ({@code @Nullable Boolean} to make field
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment should be updated too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To what? I already updated this from earlier.

Copy link
Contributor

Choose a reason for hiding this comment

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

The same as the hasLayer method? "whether or not the history object corresponds to a layer in the container"

* optional).
*/
@JsonProperty("empty_layer")
@Nullable
private Boolean emptyLayer;

public HistoryEntry() {}

public HistoryEntry(
@Nullable String creationTimestamp,
@Nullable String author,
@Nullable String createdBy,
@Nullable String comment,
@Nullable Boolean emptyLayer) {
this.author = author;
this.creationTimestamp = creationTimestamp;
this.createdBy = createdBy;
this.comment = comment;
this.emptyLayer = emptyLayer;
}

/**
* Returns whether or not the history object corresponds to a layer in the container.
*
* @return {@code true} if the history object corresponds to a layer in the container
*/
@JsonIgnore
public boolean hasLayer() {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: hasLayer seems to mean if this HistoryEntry has a layer - perhaps hasLayerInContainer or hasCorrespondingLayer or isForLayer?

return emptyLayer == null ? false : emptyLayer;
}

@Override
public boolean equals(Object other) {
if (this == other) {
return true;
}
if (other instanceof HistoryEntry) {
HistoryEntry otherHistory = (HistoryEntry) other;
return Objects.equals(otherHistory.creationTimestamp, creationTimestamp)
&& Objects.equals(otherHistory.author, author)
&& Objects.equals(otherHistory.createdBy, createdBy)
&& Objects.equals(otherHistory.comment, comment)
&& Objects.equals(otherHistory.emptyLayer, emptyLayer);
}
return false;
}

@Override
public int hashCode() {
return Objects.hash(author, creationTimestamp, createdBy, comment, emptyLayer);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,11 @@ public Blob getContainerConfigurationBlob() {
template.addLayerDiffId(layer.getDiffId());
}

// Adds the history.
for (HistoryEntry historyObject : image.getHistory()) {
template.addHistory(historyObject);
}

// Sets the creation time. Instant#toString() returns an ISO-8601 formatted string.
template.setCreated(image.getCreated() == null ? null : image.getCreated().toString());

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ public static Image<Layer> toImage(
}

List<DescriptorDigest> diffIds = containerConfigurationTemplate.getDiffIds();
List<HistoryEntry> historyObjects = containerConfigurationTemplate.getHistory();

if (layers.size() != diffIds.size()) {
throw new LayerCountMismatchException(
Expand All @@ -124,6 +125,9 @@ public static Image<Layer> toImage(

imageBuilder.addLayer(new ReferenceLayer(noDiffIdLayer.getBlobDescriptor(), diffId));
}
for (HistoryEntry historyObject : historyObjects) {
imageBuilder.addHistory(historyObject);
}

if (containerConfigurationTemplate.getCreated() != null) {
try {
Expand Down
Loading