Write out image digest to <target>/jib-image.digest #1155

briandealwis · 2018-10-16T19:57:15Z

Fixed #933

briandealwis · 2018-10-16T19:57:36Z

I'm not enamoured by this solution which has a lot of repetition. I'll look into instead leveraging the eventing system.

coollog

I think we can just refactor the writing of the digest out to BuildStepsRunner or somewhere else in jib-plugins-common?

coollog · 2018-10-16T20:01:07Z

jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildTarTask.java

+  }
+
+  /**
+   * Returns the output directory for the image digest. By default, it is {@code


output file

jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildTarTask.java

coollog · 2018-10-16T20:03:38Z

...ven-plugin/src/test/java/com/google/cloud/tools/jib/maven/BuildImageMojoIntegrationTest.java

@@ -117,6 +132,8 @@ private static String buildAndRunAdditionalTag(
    verifier.executeGoals(Arrays.asList("clean", "compile", "jib:build"));
    verifier.verifyErrorFreeLog();

+    assertImageDigest(projectRoot);


For the test, we can actually pull and build using the digest that was generated to assert that the image was actually pushed with that digest.

briandealwis · 2018-10-18T15:46:52Z

So I tried various other approaches and creating an ImageCreatedEvent seemed the cleanest approach IMHO.

coollog · 2018-10-18T16:25:02Z

Hmm, what was the reason for this extra ImageCreatedEvent when the output of the build is already the image digest?

briandealwis · 2018-10-18T19:06:41Z

The additional information is what I'd want from the library. From my experience, the image digest is only useful for the build to registry case: I can't use it to access the created image In the build to docker case (nor build to tar). I can't look up a container by the image digest if I haven't pushed it to a registry. I can't use the image digest to run the image.

An alternative is to change the return results from a DescriptorDigest to a JibContainer and add more information to JibContainer.

Why an event? It allowed me to separate my action (write the digest to a file) from when that information was available. I could have modified BuildStepRunner's forBuild* methods to pass along the path to write to, or I could have added yet another argument to BuildStepsRunner.build(). But that doesn't scale — what if we want to write out the Image ID too? I could have passed along a file name additional methods on BuildStepsRunner.

BuildStepsRunner.forBuildImage(targetImageReference, jibExtension.getTo().getTags())
        .setImageDigestPath(imageDigestPath)
        .build(
            jibContainerBuilder,
            containerizer,
            eventDispatcher,
            gradleProjectProperties.getJavaLayerConfigurations().getLayerConfigurations(),
            helpfulSuggestions);

coollog

Yes, that's a good point that the image digest is only able to reference the image in the build to registry case. However, I'm not sure about the necessity of an event handler to write the file. I think adding the event might be redundant (compared to just adding more information to JibContainer) since then both JibContainer and the ImageCreatedEvent would essentially have the same information. Also, event handling is a bit limited in what can be actually done since, for one, it doesn't currently have a good way of throwing an exception since it is executed asynchronously, whereas if we used JibContainer to get the information after the build executes, we could write out the digest file synchronously and throw an exception if it did not succeed. The current way the eventing system is designed is more suited for tracking what is actually being done by the asynchronous steps rather than performing any costly operations or file I/O, but we could potentially change that (such as allowing checked exceptions to be thrown by event handlers).

coollog · 2018-10-18T19:48:52Z

jib-core/src/main/java/com/google/cloud/tools/jib/event/events/ImageCreatedEvent.java

+  }
+
+  /** @return the created image */
+  public Image<? extends Layer> getImage() {


I think we can remove this since Image is more of an internal representation rather than something we want to expose to a user.

coollog · 2018-10-18T19:51:40Z

jib-core/src/main/java/com/google/cloud/tools/jib/event/events/ImageCreatedEvent.java

+  }
+
+  /**
+   * Return the <em>image digest</em>, the digest of the registry image manifest.


Ah right, maybe we should use <strong> and <em> in our other javadocs as well.

coollog · 2018-10-18T19:52:49Z

jib-gradle-plugin/src/integration-test/java/com/google/cloud/tools/jib/gradle/JibRunHelper.java

+      String digest = new String(Files.readAllBytes(digestPath), StandardCharsets.UTF_8);
+      DescriptorDigest.fromDigest(digest);
+    } catch (IOException | DigestException ex) {
+      throw new AssertionError("Invalid jib-image.digest", ex);


I think since this is a test we can just have this method throw the exceptions directly?

coollog · 2018-10-18T19:53:24Z

jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/GradleProjectProperties.java

+                new TimerEventHandler(message -> logEventHandler.accept(LogEvent.debug(message))));
+
+    Path buildOutputPath = project.getBuildDir().toPath();
+    Path digestOutputPath = buildOutputPath.resolve("jib-image.digest");


I think we should make this configurable like jib-image.tar is?

It's not? https://github.com/GoogleContainerTools/jib/blob/master/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildTarTask.java#L100

Oh interesting, I thought we had that as configurable - nevermind then.

briandealwis

Reverted back and modified BuildStepsRunner and friends to return a JibContainer. Augmented the JibContainer with both image digest and ID. Added support for writing out the imageDigest to BuildStepsRunner.

briandealwis · 2018-10-19T21:04:21Z

jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/GradleProjectProperties.java

+                new TimerEventHandler(message -> logEventHandler.accept(LogEvent.debug(message))));
+
+    Path buildOutputPath = project.getBuildDir().toPath();
+    Path digestOutputPath = buildOutputPath.resolve("jib-image.digest");


It's not? https://github.com/GoogleContainerTools/jib/blob/master/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildTarTask.java#L100

coollog · 2018-10-22T17:06:02Z

jib-core/src/main/java/com/google/cloud/tools/jib/api/JibContainer.java

+   * @param imageId digest of the container configuration
+   * @return the new container
+   */
+  public static JibContainer create(DescriptorDigest imageDigest, DescriptorDigest imageId) {


Hmm, minor API thing, but since JibContainer is the user-facing API for representing the build results, it might be better to not include a public factory method to create a JibContainer. I think we should still keep the package-private constructor (or a package-private factory method) to only allow JibContainerBuilder#containerize to instantiate it.

I don't disagree, but it's created from the build steps in .build.steps, not .api.

I've moved the method on JibContainerBuilder and added a note that it is for internal consumption. I could mark it with Guava's com.google.common.annotations.Beta annotation.

I think we should keep the package-hierarchy free of cyclical dependencies such that .api depends on .build.steps and there isn't a reference in the other direction - so an alternative here would be to have the build steps return the image digest and id (via some class like BuildSteps.Result or something) and have JibContainerBuilder create the JibContainer using those return values.

jib-core/src/main/java/com/google/cloud/tools/jib/api/JibContainer.java

jib-core/src/test/java/com/google/cloud/tools/jib/api/JibContainerTest.java

coollog · 2018-10-22T17:18:09Z

jib-gradle-plugin/src/integration-test/java/com/google/cloud/tools/jib/gradle/JibRunHelper.java

@@ -130,15 +130,11 @@ static void assertCreationTimeEpoch(String imageReference)
        new Command("docker", "inspect", "-f", "{{.Created}}", imageReference).run().trim());
  }

-  static void assertImageDigest(Path projectRoot) {
+  static String assertImageDigest(Path projectRoot) throws IOException, DigestException {


Is the String return used?

Hmm no. We're testing the result in the maven tests, so I'll drop it from here.

coollog · 2018-10-22T17:22:29Z

jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildImageMojo.java

      BuildStepsRunner.forBuildImage(targetImageReference, getTargetImageAdditionalTags())
+          .imageDigestOutputPath(imageDigestOutputPath)


Hmm, rather than a nullable with a setter, could this just be writeImageDigest(path) method on either a separate class (like ImageDigestWriter?) or chained onto BuildStepsRunner#build?

Like

BuildStepsRunner... .build(...) .writeImageDigest(imageDigestOutputPath);

jib-core/src/main/java/com/google/cloud/tools/jib/api/JibContainer.java

jib-core/src/main/java/com/google/cloud/tools/jib/api/JibContainerBuilder.java

TadCordle

Aside from Q's comments/a few javadoc nits, seems good to me.

TadCordle · 2018-10-26T14:43:03Z

jib-core/src/main/java/com/google/cloud/tools/jib/api/JibContainer.java

  }

  /**
-   * Gets the digest of the container image built by Jib.
+   * Gets the image digest, the digest of the registry image manifest built by Jib. This digest can


First sentence seems a bit redundant, maybe just "Gets the digest of the registry image manifest built by Jib."

TadCordle · 2018-10-26T14:43:24Z

jib-core/src/main/java/com/google/cloud/tools/jib/api/JibContainer.java

   *
   * @return the image digest
   */
  public DescriptorDigest getDigest() {
    return imageDigest;
  }
+
+  /**
+   * Gets the image ID, the digest of the container configuration built by Jib.


Same here, "Gets the digest of the container configuration built by Jib."

TadCordle · 2018-10-26T14:49:39Z

jib-core/src/main/java/com/google/cloud/tools/jib/builder/BuildSteps.java

@@ -35,13 +35,13 @@
  private interface ImageBuildRunnable {

    /**
-     * Builds an image and returns its digest.
+     * Build an image.


This should still be "Builds" with an 's', shouldn't it?

coollog · 2018-10-26T22:31:46Z

Let's add a CHANGLOG entry as well.

TadCordle · 2018-10-29T18:54:08Z

jib-core/src/main/java/com/google/cloud/tools/jib/builder/steps/BuildResult.java

+
+  @Override
+  public boolean equals(Object obj) {
+    if (this == obj) return true;


nit: I think we generally still use brackets for single-line if-statements.

coollog

LGTM, just some merge conflicts to resolve.

coollog · 2018-10-29T19:09:08Z

jib-core/src/main/java/com/google/cloud/tools/jib/builder/steps/BuildResult.java

+  public boolean equals(Object obj) {
+    if (this == obj) return true;
+    if (obj == null) return false;
+    if (getClass() != obj.getClass()) return false;


I think we generally use instanceof even though we don't use inheritance, but it does save the extra null check.

I meant to look up Bloch previously and he discusses at length in Effective Java how .equals() is required to be symmetric and thus instanceof should never be used.

Filed #1202

Write out image digest to <target>/jib-image.digest

90a33bd

googlebot added the cla: yes label Oct 16, 2018

coollog added the in progress label Oct 16, 2018

coollog reviewed Oct 16, 2018

View reviewed changes

briandealwis added 2 commits October 18, 2018 10:20

Use ImageCreatedEvent to write out image file

2317301

Re-execute test images using image digest

608913e

coollog reviewed Oct 18, 2018

View reviewed changes

briandealwis added 3 commits October 19, 2018 16:27

Merge remote-tracking branch 'origin/master' into HEAD

a7ca41c

Revert and expand JibContainer instead

cd083d5

Change assertImageDigest to not suppress underlying exceptions

c0ff330

briandealwis commented Oct 20, 2018

View reviewed changes

briandealwis added 2 commits October 22, 2018 09:35

javadoc + formatting

0a06115

Remove unnecessary stubbing; add javadoc

a68f286

coollog reviewed Oct 22, 2018

View reviewed changes

review

074e38e

briandealwis force-pushed the i933 branch from cf08f48 to 074e38e Compare October 22, 2018 19:06

coollog reviewed Oct 22, 2018

View reviewed changes

jib-core/src/main/java/com/google/cloud/tools/jib/api/JibContainer.java Outdated Show resolved Hide resolved

jib-core/src/main/java/com/google/cloud/tools/jib/api/JibContainerBuilder.java Outdated Show resolved Hide resolved

coollog reviewed Oct 22, 2018

View reviewed changes

jib-core/src/main/java/com/google/cloud/tools/jib/api/JibContainerBuilder.java Outdated Show resolved Hide resolved

TadCordle reviewed Oct 26, 2018

View reviewed changes

rework back to a method on BuildStepsRunner

5e67d56

briandealwis added 4 commits October 29, 2018 14:02

Add changelog, fix tests

3cf267d

Remove redundant terms

e95cb3e

Merge with HEAD

21b5818

Use same tense

833c340

TadCordle approved these changes Oct 29, 2018

View reviewed changes

coollog reviewed Oct 29, 2018

View reviewed changes

briandealwis added 2 commits October 29, 2018 16:23

equals

a2268c9

Merge with HEAD

118485b

briandealwis mentioned this pull request Oct 29, 2018

Make equals() symmetric #1203

Closed

briandealwis merged commit 8f46e19 into master Oct 29, 2018

briandealwis deleted the i933 branch November 23, 2018 15:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Write out image digest to <target>/jib-image.digest #1155

Write out image digest to <target>/jib-image.digest #1155

briandealwis commented Oct 16, 2018

briandealwis commented Oct 16, 2018

coollog left a comment

coollog Oct 16, 2018

coollog Oct 16, 2018

briandealwis commented Oct 18, 2018

coollog commented Oct 18, 2018

briandealwis commented Oct 18, 2018

coollog left a comment

coollog Oct 18, 2018

coollog Oct 18, 2018

coollog Oct 18, 2018

coollog Oct 18, 2018

briandealwis Oct 19, 2018

coollog Oct 22, 2018

briandealwis left a comment

briandealwis Oct 19, 2018

coollog Oct 22, 2018

briandealwis Oct 22, 2018

coollog Oct 22, 2018

coollog Oct 22, 2018

briandealwis Oct 22, 2018

coollog Oct 22, 2018 •

edited

Loading

TadCordle left a comment

TadCordle Oct 26, 2018

TadCordle Oct 26, 2018

TadCordle Oct 26, 2018

coollog commented Oct 26, 2018

TadCordle Oct 29, 2018

coollog left a comment

coollog Oct 29, 2018

briandealwis Oct 29, 2018

briandealwis Oct 29, 2018

		BuildStepsRunner.forBuildImage(targetImageReference, getTargetImageAdditionalTags())
		.imageDigestOutputPath(imageDigestOutputPath)

Write out image digest to <target>/jib-image.digest #1155

Write out image digest to <target>/jib-image.digest #1155

Conversation

briandealwis commented Oct 16, 2018

briandealwis commented Oct 16, 2018

coollog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briandealwis commented Oct 18, 2018

coollog commented Oct 18, 2018

briandealwis commented Oct 18, 2018

coollog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briandealwis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coollog Oct 22, 2018 • edited Loading

Choose a reason for hiding this comment

TadCordle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coollog commented Oct 26, 2018

Choose a reason for hiding this comment

coollog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coollog Oct 22, 2018 •

edited

Loading