Refactor configuration build/push/tar build steps to reduce duplicate code #269

TadCordle · 2018-05-11T19:05:11Z

Part of #262 (refactors BuildContainerConfigurationStep, BuildTarballLoadDockerStep, and PushImageStep).

Fixes #263 (adds ability to add blobs to TarStreamBuilder without temp files).

…lLoadDockerSteps, and PushImageStep; allow adding blobs to TarStreamBuilder

coollog · 2018-05-11T19:49:06Z

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

@@ -32,7 +30,7 @@
 import java.util.concurrent.Future;

 /** Builds the container configuration. */
-class BuildContainerConfigurationStep implements Callable<ListenableFuture<Blob>> {
+class BuildContainerConfigurationStep implements Callable<ListenableFuture<Image>> {


BuildImageStep now?

coollog · 2018-05-11T19:50:36Z

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

-                      pushBaseImageLayerFuturesFuture, pushContainerConfigurationFutureFuture)
+                      pushBaseImageLayerFuturesFuture,
+                      pushContainerConfigurationFutureFuture,
+                      buildContainerConfigurationFutureFuture)


The dependency on buildContainerConfigurationFutureFuture shouldn't be necessary since pushContainerConfigurationFutureFuture already depends on it.

coollog · 2018-05-11T20:04:04Z

jib-core/src/main/java/com/google/cloud/tools/jib/tar/TarStreamBuilder.java

@@ -61,6 +68,17 @@ public void addEntry(TarArchiveEntry entry) {
    entries.add(entry);
  }

+  /** Adds a blob to the archive. */
+  public void addEntry(Blob blob, String name) throws IOException {
+    // TODO: Efficiency? We're writing blob contents twice (once here to get entry size, once when


Yeah, I think you could do it by creating a class that encompasses either a file or a blob and a name, and it has a method to write it to a TarArchiveOutputStream with the right TarArchiveEntry and contents.

coollog · 2018-05-11T21:36:07Z

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

@@ -29,8 +29,8 @@
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Future;

-/** Builds the container configuration. */
-class BuildContainerConfigurationStep implements Callable<ListenableFuture<Image>> {
+/** Builds the container image. */


This is actually the {@link Image} class, which is a model for the container image.

coollog · 2018-05-11T21:38:06Z

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

@@ -87,18 +87,19 @@ private Void afterPushBaseImageLayerFuturesFuture()
    DockerLoadManifestTemplate manifestTemplate = new DockerLoadManifestTemplate();
    for (Layer layer : image.getLayers()) {
      String layerName = layer.getBlobDescriptor().getDigest() + ".tar.gz";
-      tarStreamBuilder.addEntry(layer.getBlob(), layerName);
+      tarStreamBuilder.addEntry(Blobs.writeToByteArray(layer.getBlob()), layerName);


This should use the cached content file so that we're not storing the files in memory.

coollog · 2018-05-11T21:39:00Z

jib-core/src/main/java/com/google/cloud/tools/jib/tar/TarStreamBuilder.java

-    // TODO: Efficiency? We're writing blob contents twice (once here to get entry size, once when
-    // writing to TarArchiveOutputStream).
-    byte[] bytes = Blobs.writeToByteArray(blob);
+  public void addEntry(byte[] contents, String name) {


I think we generally want to avoid using a raw byte[], so String would prob be better here.

coollog · 2018-05-14T14:34:59Z

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

@@ -145,10 +144,10 @@ public void run()

          timer2.lap("Setting up build container configuration");
          // Builds the container configuration.
-          ListenableFuture<ListenableFuture<Blob>> buildContainerConfigurationFutureFuture =
+          ListenableFuture<ListenableFuture<Image>> buildContainerConfigurationFutureFuture =


s/buildContainerConfigurationFutureFuture/buildImageFutureFuture

coollog · 2018-05-14T14:36:16Z

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

  private final BuildConfiguration buildConfiguration;
  private final ListeningExecutorService listeningExecutorService;
  private final ListenableFuture<List<ListenableFuture<CachedLayer>>>
      pullBaseImageLayerFuturesFuture;
  private final List<ListenableFuture<CachedLayer>> buildApplicationLayerFutures;
-  private final ListenableFuture<ListenableFuture<Blob>> buildConfigurationFutureFuture;
+  private final ListenableFuture<ListenableFuture<Image>> buildConfigurationFutureFuture;


buildImageFutureFuture

coollog · 2018-05-14T14:36:33Z

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


  BuildTarballAndLoadDockerStep(
      BuildConfiguration buildConfiguration,
      ListeningExecutorService listeningExecutorService,
      ListenableFuture<List<ListenableFuture<CachedLayer>>> pullBaseImageLayerFuturesFuture,
      List<ListenableFuture<CachedLayer>> buildApplicationLayerFutures,
-      ListenableFuture<ListenableFuture<Blob>> buildConfigurationFutureFuture) {
+      ListenableFuture<ListenableFuture<Image>> buildConfigurationFutureFuture) {


buildImageFutureFuture

coollog · 2018-05-14T14:47:30Z

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

@@ -124,10 +123,10 @@ public void run()

          timer2.lap("Setting up build container configuration");
          // Builds the container configuration.
-          ListenableFuture<ListenableFuture<Blob>> buildContainerConfigurationFutureFuture =
+          ListenableFuture<ListenableFuture<Image>> buildContainerConfigurationFutureFuture =


buildImageFutureFuture

coollog · 2018-05-14T14:49:25Z

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

+    DockerLoadManifestTemplate manifestTemplate = new DockerLoadManifestTemplate();
+
+    for (Layer layer : image.getLayers()) {
+      Path cachedFile = ((CachedLayer) layer).getContentFile();


I feel like we should actually change Image to only hold and return CachedLayers so that we don't have to do the cast here, but that change could prob go in a TODO/separate issue/PR.

coollog · 2018-05-14T14:55:47Z

jib-core/src/main/java/com/google/cloud/tools/jib/tar/TarStreamBuilder.java

-  /** Writes each entry in the filesystem to the tarball archive stream. */
-  private static void writeEntriesAsTarArchive(
-      List<TarArchiveEntry> entries, OutputStream tarByteStream) throws IOException {
+  /** Map of TarArchiveEntries and their corresponding content as output stream consumers. */


Maybe phrase like:

Maps from {@link TarArchiveEntry} to function that outputs the entry onto a {@link TarArchiveOutputStream}. The order of the entries is the order they belong in the tarball.

coollog · 2018-05-14T14:57:58Z

jib-core/src/main/java/com/google/cloud/tools/jib/tar/TarStreamBuilder.java

-  private static void writeEntriesAsTarArchive(
-      List<TarArchiveEntry> entries, OutputStream tarByteStream) throws IOException {
+  /** Map of TarArchiveEntries and their corresponding content as output stream consumers. */
+  private final LinkedHashMap<TarArchiveEntry, TarArchiveOutputStreamConsumer> blobContents =


archiveMap?

coollog · 2018-05-14T15:01:23Z

jib-core/src/main/java/com/google/cloud/tools/jib/tar/TarStreamBuilder.java

+        tarArchiveOutputStream -> {
+          if (entry.isFile()) {
+            try (InputStream contentStream =
+                new BufferedInputStream(new FileInputStream(entry.getFile()))) {


Files.newInputStream

coollog · 2018-05-14T15:13:11Z

jib-core/src/main/java/com/google/cloud/tools/jib/tar/TarStreamBuilder.java

+  }
+
+  /** Adds a blob to the archive. */
+  public void addEntry(String contents, String name) {


Should add a test for this case.

coollog · 2018-05-14T15:18:41Z

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

-        tarStreamBuilder.addEntry(
-            new TarArchiveEntry(layerFile.toFile(), layerFile.getFileName().toString()));
-      }
+    DockerLoadManifestTemplate manifestTemplate = new DockerLoadManifestTemplate();


Prob make a TODO/file an issue to refactor this tar building to a separate class specifically for building a Docker image tar so that it is unit-testable. Generally, these async steps should be kept as minimal as possible.

Looks like #261 is this.

coollog · 2018-05-14T17:23:20Z

jib-core/src/test/java/com/google/cloud/tools/jib/tar/TarStreamBuilderTest.java

+  public void testToBlob_tarArchiveEntries() throws IOException {
+    setUpWithTarEntries();
+
+    Blob blob = testTarStreamBuilder.toBlob();


Looks like all these verification methods are the same for each test - can put right into verifyTarArchive.

coollog · 2018-05-14T17:23:53Z

jib-core/src/test/java/com/google/cloud/tools/jib/tar/TarStreamBuilderTest.java

+  }
+
+  /** Creates a TarStreamBuilder using Strings. */
+  private void setUpWithStrings() {


One more with both files and strings would be good.

coollog

Good to go! 👍

coollog · 2018-05-14T19:29:14Z

jib-core/src/test/java/com/google/cloud/tools/jib/tar/TarStreamBuilderTest.java

+  }
+
+  /** Creates a compressed blob from the TarStreamBuilder and verifies it. */
+  private void verifyBlobWithCompression() throws IOException {


This is fine since it's just a test helper, but since theres only two line difference between these functions, it could just be one function with a parameter "compress". Then, like line 140 could be blob.writeTo(compress?new GZIPOutputStream(tarByteOutputStream):tarByteOutputStream).

TadCordle added 2 commits May 11, 2018 14:56

Reduce duplicate code in BuildContainerConfigurationStep, BuildTarbal…

0a6d4d1

…lLoadDockerSteps, and PushImageStep; allow adding blobs to TarStreamBuilder

template -> manifestTemplate

1535b0f

TadCordle added the area/jib-core label May 11, 2018

TadCordle requested review from loosebazooka and coollog May 11, 2018 19:05

coollog reviewed May 11, 2018

View reviewed changes

Feedback + refactor further

9f16937

coollog reviewed May 11, 2018

View reviewed changes

Feedback

7e315ca

coollog reviewed May 14, 2018

View reviewed changes

TadCordle added 2 commits May 14, 2018 13:06

Rename everything

610d840

Add tests for TarStreamBuilder.addEntry(String, String)

200d42c

coollog reviewed May 14, 2018

View reviewed changes

More tests

428f565

coollog approved these changes May 14, 2018

View reviewed changes

TadCordle merged commit 50ad775 into master May 14, 2018

TadCordle deleted the refactor_config_steps branch May 14, 2018 19:59

TadCordle mentioned this pull request May 23, 2018

Refactor to reduce duplicate code #262

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor configuration build/push/tar build steps to reduce duplicate code #269

Refactor configuration build/push/tar build steps to reduce duplicate code #269

TadCordle commented May 11, 2018

coollog May 11, 2018

coollog May 11, 2018

coollog May 11, 2018

coollog May 11, 2018

coollog May 11, 2018

coollog May 11, 2018

coollog May 14, 2018

coollog May 14, 2018

coollog May 14, 2018

coollog May 14, 2018

coollog May 14, 2018

coollog May 14, 2018

coollog May 14, 2018

coollog May 14, 2018

coollog May 14, 2018

coollog May 14, 2018

coollog May 14, 2018

coollog May 14, 2018

coollog May 14, 2018

coollog left a comment

coollog May 14, 2018

Refactor configuration build/push/tar build steps to reduce duplicate code #269

Refactor configuration build/push/tar build steps to reduce duplicate code #269

Conversation

TadCordle commented May 11, 2018

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

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

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment