Write history as part of layer metadata #877
Conversation
TadCordle
requested review from
briandealwis,
loosebazooka,
coollog and
chanseokoh
| Image<Layer> baseImage = NonBlockingSteps.get(pullBaseImageStep).getBaseImage(); | ||
| ContainerConfiguration containerConfiguration = | ||
| buildConfiguration.getContainerConfiguration(); | ||
| String buildTime = |
There was a problem hiding this comment.
nit: creationTime may be less ambiguous - buildTime can be construed as time it took to build
| imageBuilder.addEnvironment(baseImage.getEnvironment()); | ||
| imageBuilder.addLabels(baseImage.getLabels()); | ||
|
|
||
| ContainerConfiguration containerConfiguration = | ||
| buildConfiguration.getContainerConfiguration(); | ||
| // Add history elements if base layers/history sizes don't match |
There was a problem hiding this comment.
This can be used to add the application layer history objects too - just add the buildAndCacheApplicationLayerSteps.size(). This way, there's only one place that calls new HistoryObjectTemplate(buildTime, "Jib", "jib")
| * { | ||
| * "author": "Jib", | ||
| * "created": "1970-01-01T00:00:00Z", | ||
| * "created_by": "jib build ..." |
There was a problem hiding this comment.
ah this is my bad, the bug description is off, I'll go update this.
| @@ -139,5 +143,9 @@ public void test_propagateBaseImageConfiguration() | |||
| Assert.assertEquals( | |||
| ImmutableMap.of("base.label", "base.label.value", "my.label", "my.label.value"), | |||
| image.getLabels()); | |||
| Assert.assertTrue( | |||
| "History doesn't contain expected base image layer", | |||
| image.getHistory().contains(expectedHistory)); | |||
There was a problem hiding this comment.
Assert the first history is the expectedHistory?
| @Nullable private String created_by; | ||
|
|
||
| /** Whether or not the layer is empty */ | ||
| @Nullable private Boolean empty_layer; |
There was a problem hiding this comment.
boolean? I believe we can default to false.
There was a problem hiding this comment.
I wanted to make this field optional, so I made it a @Nullable Boolean instead of boolean (if it's boolean then the field will always show up in the generated config).
There was a problem hiding this comment.
Ah right, that makes sense 👍
| } | ||
|
|
||
| @Override | ||
| public boolean equals(Object other) { |
There was a problem hiding this comment.
nit: can short circuit with if (this == other) return true;
| // Add history elements if base layers/history sizes don't match | ||
| if (baseImage.getHistory().size() < baseImageLayers.size()) { | ||
| int sizeDifference = baseImageLayers.size() - baseImage.getHistory().size(); | ||
| for (int count = 0; count < sizeDifference; count++) { |
There was a problem hiding this comment.
History with empty_layer=true should not be taken into account when calculating the history objects to add (they don't correspond to a layer). This should also get a test to make sure this is accounted for correctly.
| import javax.annotation.Nullable; | ||
|
|
||
| @JsonIgnoreProperties(ignoreUnknown = true) | ||
| public class HistoryObjectTemplate implements JsonTemplate { |
There was a problem hiding this comment.
Also, was there a reason for having this outside of ContainerConfigurationTemplate (besides just a long file)? If we keep it outside, might want to name it like ContainerConfigurationHistoryObjectTemplate for something.
There was a problem hiding this comment.
Wasn't sure if it was a good idea to use nested classes in this case since HistoryObjectTemplate is used in multiple places outside of ContainerConfigurationTemplate. But I could nest it if you think it's a better idea.
There was a problem hiding this comment.
It's mostly that even though it is used outside of ContainerConfigurationTemplate, it is just a type of object inside the config JSON, so some sort of association there that ties them closely would probably make more sense for the class relationship.
There was a problem hiding this comment.
Ok, I will move it to be inside ContainerConfigurationTemplate.
| baseImageLayers.size() + buildAndCacheApplicationLayerSteps.size() - nonEmptyLayerCount; | ||
| for (int count = 0; count < sizeDifference; count++) { | ||
| imageBuilder.addHistory( | ||
| new HistoryObjectTemplate( |
There was a problem hiding this comment.
Oh right, I just realized that this was passing a JSON template object into the Image builder. Image is intended to be a canonicalized Java object model of an image independent of the format (hence why we have ImageToJsonTranslator/JsonToImageTranslator). Making this distinction would allow for this image-building code not have to touch JSON templates at all.
There was a problem hiding this comment.
Is there a way to address this without making two nearly identical classes?
There was a problem hiding this comment.
Hmm actually I guess they might not be that identical. It still doesn't feel good to be making two classes that hold exactly the same values, the only difference being that one is for serialization.
| return empty_layer == null ? false : empty_layer; | ||
| } | ||
|
|
||
| @VisibleForTesting |
|
fwiw, this issue is causing problems for me with a private repo. The owner has confirmed that your fix addresses that issue. |
| // Add history elements for non-empty layers that don't have one yet | ||
| int sizeDifference = | ||
| baseImageLayers.size() + buildAndCacheApplicationLayerSteps.size() - nonEmptyLayerCount; | ||
| for (int count = 0; count < sizeDifference; count++) { |
There was a problem hiding this comment.
My only concern here is that we're claiming (and setting ourselves up for blame for) layers that were not generated by us. Perhaps we could only add missing-history items here, and then add jib layers in the next for() where you're adding the Jib-generated layers (lines 136–139)?
There was a problem hiding this comment.
Yeah, I originally had it this way and changed it so we only needed to use new HistoryItem() once, but on second thought I agree it's clearer the original way (easier to tell which history entries correspond to which layers).
|
|
||
| /** Represents an item in the container configuration's {@code history} list. */ | ||
| @JsonIgnoreProperties(ignoreUnknown = true) | ||
| public class HistoryItem implements JsonTemplate { |
There was a problem hiding this comment.
Or HistoryEntry? (Just throwing this out as a suggestion.)
| @Nullable private String created_by; | ||
|
|
||
| /** Whether or not the layer is empty ({@code @Nullable Boolean} to make field optional). */ | ||
| @Nullable private Boolean empty_layer; |
There was a problem hiding this comment.
Question: should we follow Google naming style and use @JsonProperty for the external?
| ? Instant.EPOCH.toString() | ||
| : containerConfiguration.getCreationTime().toString(); | ||
| for (int count = 0; count < baseImageLayers.size() - nonEmptyLayerCount; count++) { | ||
| imageBuilder.addHistory(new HistoryEntry(layerCreationTime, "Jib", "jib", null)); |
There was a problem hiding this comment.
But I don't think this should be "Jib"/"jib" — maybe author = "(unknown)" and createdBy= "(unavailable)"?
There was a problem hiding this comment.
I can change them, but right now createdBy is just a placeholder. After this I think I'm going to add a way to pass in something like "jib-gradle-plugin" or "jib-maven-plugin". Maybe I should do the same for author?
@coollog WDYT
There was a problem hiding this comment.
I think Brian means for the base image layers that don't have history to have author and createdBy be (unknown), though I think leaving them empty would be fine too.
There was a problem hiding this comment.
Oh ok, makes sense.
| public class HistoryEntry implements JsonTemplate { | ||
|
|
||
| /** The timestamp at which the image was created. */ | ||
| private String created; |
There was a problem hiding this comment.
Do we want to be making the created field non-nullable (in the config spec, it is optional)?
There was a problem hiding this comment.
I was referencing this and it didn't seem optional (no "omitempty").
There was a problem hiding this comment.
Hmm, interesting - okay, let's match that behavior then since we don't want to break compatibility with Docker.
There was a problem hiding this comment.
But on the flip side we may error out reading an OCI-conformant image that has no created field. Right?
| public class HistoryEntry implements JsonTemplate { | ||
|
|
||
| /** The timestamp at which the image was created. */ | ||
| private String created; |
There was a problem hiding this comment.
Can name this in code style and use JsonProperty here too. Something like creationTimestamp is more descriptive.
| @JsonIgnoreProperties(ignoreUnknown = true) | ||
| public class HistoryEntry implements JsonTemplate { | ||
|
|
||
| /** The timestamp at which the image was created. */ |
There was a problem hiding this comment.
A comment about this needing to be formatted according to RFC 3339, section 5.6
| @Nullable | ||
| private String createdBy; | ||
|
|
||
| /** Whether or not the layer is empty ({@code @Nullable Boolean} to make field optional). */ |
There was a problem hiding this comment.
This field is actually about whether or not this history entry corresponds to an actual layer in the container.
| /** The name of the author specified when committing the image. */ | ||
| @Nullable private String author; | ||
|
|
||
| /** The command used while building the image. */ |
There was a problem hiding this comment.
s/while building/to build/?
| import java.util.Objects; | ||
| import javax.annotation.Nullable; | ||
|
|
||
| /** Represents an item in the container configuration's {@code history} list. */ |
There was a problem hiding this comment.
Let's add a link to https://github.com/opencontainers/image-spec/blob/master/config.md#properties and pointing specifically to the history field.
|
|
||
| /** Represents an item in the container configuration's {@code history} list. */ | ||
| @JsonIgnoreProperties(ignoreUnknown = true) | ||
| public class HistoryEntry implements JsonTemplate { |
There was a problem hiding this comment.
I think this might better belong under the image.json package, next to ContainerConfigurationTemplate.
I'm thinking it might make more sense to call this ContainerConfigurationHistory and have it manage the actual JSON templates. Then, Image will have a ContainerConfigurationHistory and not have to directly manage HistoryEntry JSON templates.
There was a problem hiding this comment.
I'll move it to the package, but I'm just going to make an issue for the second point for now.
| /** Whether or not the layer is empty ({@code @Nullable Boolean} to make field optional). */ | ||
| @JsonProperty("empty_layer") | ||
| @Nullable | ||
| private Boolean emptyLayer; |
There was a problem hiding this comment.
Could be named like isLayer?
There was a problem hiding this comment.
I think that naming is too different from the field it corresponds to and could get confusing.
|
Might be worth including the |
| @@ -124,14 +124,15 @@ | |||
| ? Instant.EPOCH.toString() | |||
| : containerConfiguration.getCreationTime().toString(); | |||
| for (int count = 0; count < baseImageLayers.size() - nonEmptyLayerCount; count++) { | |||
| imageBuilder.addHistory(new HistoryEntry(layerCreationTime, null, null, null)); | |||
| imageBuilder.addHistory( | |||
| new HistoryEntry(layerCreationTime, null, null, "auto-generated by Jib", null)); | |||
There was a problem hiding this comment.
We probably want to go with a builder pattern here since it's unclear which field is which.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
That's comment, not created_by.
There was a problem hiding this comment.
Ah right, then my first comment applies haha.
| @JsonIgnoreProperties(ignoreUnknown = true) | ||
| public class HistoryEntry implements JsonTemplate { | ||
|
|
||
| /** The RFC 3339 formatted timestamp at which the image was created. */ |
There was a problem hiding this comment.
nit: specifically section 5.6 of RFC 3339
| @Nullable | ||
| private String author; | ||
|
|
||
| /** The command used to build the image. */ |
| private String comment; | ||
|
|
||
| /** | ||
| * Whether or not the entry corresponds to an empty layer ({@code @Nullable Boolean} to make field |
There was a problem hiding this comment.
This comment should be updated too
There was a problem hiding this comment.
To what? I already updated this from earlier.
There was a problem hiding this comment.
The same as the hasLayer method? "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() { |
There was a problem hiding this comment.
nit: hasLayer seems to mean if this HistoryEntry has a layer - perhaps hasLayerInContainer or hasCorrespondingLayer or isForLayer?
| @@ -137,10 +152,18 @@ public void addLayerDiffId(DescriptorDigest diffId) { | |||
| rootfs.diff_ids.add(diffId); | |||
| } | |||
|
|
|||
| public void addHistory(HistoryEntry historyEntry) { | |||
There was a problem hiding this comment.
nit: since history is a plural list of HistoryEntrys, perhaps this method name should be addToHistory instead
| @@ -70,6 +82,9 @@ | |||
| /** 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. */ | |||
There was a problem hiding this comment.
"Describes the history of each layer"?
| @Nullable private String comment; | ||
| @Nullable private Boolean emptyLayer; | ||
|
|
||
| public Builder setCreationTimestamp(String creationTimestamp) { |
There was a problem hiding this comment.
Would it make sense to take an Instant here instead?
| @@ -105,4 +154,17 @@ public boolean equals(Object other) { | |||
| public int hashCode() { | |||
| return Objects.hash(author, creationTimestamp, createdBy, comment, emptyLayer); | |||
| } | |||
|
|
|||
| @Override | |||
| public String toString() { | |||
There was a problem hiding this comment.
Whoops, added that to test something and forgot to get rid of it.
| @@ -32,7 +32,56 @@ | |||
| @JsonIgnoreProperties(ignoreUnknown = true) | |||
| public class HistoryEntry implements JsonTemplate { | |||
|
|
|||
| /** The RFC 3339 formatted timestamp at which the image was created. */ | |||
| public static class Builder { | |||
There was a problem hiding this comment.
I didn't see any other Builders in the code base with a javadoc (and I don't think it's necessary, this is pretty self-explanatory).
|
This should fix #534 |
Part of #875. Base image history is propagated, and history elements for each of the new layers are added on top. "created_by" is always just "jib" for now; I'll submit a followup later where we can pass in different strings.