doc: cleanup documentation and formatting

dev/fattest.simplicity/src/componenttest/containers/ImageBuilder.java

@@ -41,15 +41,14 @@ class ImageBuilder {
     private static final Class<?> c = ImageBuilder.class;
 
     // The --build-arg necessary to overwrite the default BASE_IMAGE in the Dockerfile
-    // with the mirrored image in artifactory
+    // with the mirrored image in an alternative registry
     private static final String BASE_IMAGE = "BASE_IMAGE";
 
     // Image to build
     private final DockerImageName image;
 
-    // Constructor
+    // Constructor - builder class
     private ImageBuilder(DockerImageName image) {
-        //builder class
         this.image = image;
     }
 
@@ -59,10 +58,11 @@ private ImageBuilder(DockerImageName image) {
      * The Dockerfile with instructions on how to build this image must be saved in source control in directory
      * io.openliberty.org.testcontainers/resources/openliberty/testcontainers/<image-name>/<image-version>/Dockerfile
      *
-     * Note: The resulting image will be cached with name "localhost/openliberty/testcontainers/<image-name>:<image-version>"
+     * Note: The resulting image will be cached with the name "localhost/openliberty/testcontainers/<image-name>:<image-version>"
      * therefore, you must update the image version whenever a change is made to the corresponding Dockerfile.
      *
-     * @param  customImage the image to build in format "<image-name>:<image-version>" or "openliberty/testcontainers/<image-name>:<image-version>"
+     * @param  customImage the image to build in the format "<image-name>:<image-version>"
+     *                         or "openliberty/testcontainers/<image-name>:<image-version>"
      *
      * @return             instance of ImageBuilder
      */
@@ -78,6 +78,7 @@ public static ImageBuilder build(String customImage) {
      * Termination point of this builder class.
      *
      * We will first attempt to find a cached version of the image,
+     * if unsuccessful, we will attempt to pull the image from a registry,
      * if unsuccessful, we will then build the image from the Dockerfile.
      *
      * @return RemoteDockerImage that points to a cached or built image.
@@ -90,16 +91,14 @@ public RemoteDockerImage get() {
      * Helper method, attempts to find a cached version of the image.
      */
     private Optional<RemoteDockerImage> getCached() {
-        final String m = "getLocalCache";
-
-        RemoteDockerImage cachedImage = new RemoteDockerImage(image);
+        final String m = "getCached";
 
         if (PullPolicy.defaultPolicy().shouldPull(image)) {
-            Log.info(c, m, "Unable to find cached image " + image.asCanonicalNameString());
+            Log.info(c, m, "Unable to find cached image: " + image.asCanonicalNameString());
             return Optional.empty();
         } else {
-            Log.info(c, m, "Found cached image " + image.asCanonicalNameString());
-            return Optional.of(cachedImage);
+            Log.info(c, m, "Found cached image: " + image.asCanonicalNameString());
+            return Optional.of(new RemoteDockerImage(image));
         }
     }
 
@@ -110,18 +109,18 @@ private Optional<RemoteDockerImage> pullCached() {
         final String m = "pullCached";
 
         if (image.getRegistry().equalsIgnoreCase("localhost")) {
-            Log.info(c, m, "Assumed we cannot pull image from " + image.asCanonicalNameString());
+            Log.info(c, m, "Did not attempt to pull cached image from localhost for image: " + image.asCanonicalNameString());
             return Optional.empty();
         }
 
         RemoteDockerImage cachedImage = new RemoteDockerImage(image);
 
         try {
             cachedImage.get();
-            Log.info(c, m, "Found pullable image " + image.asCanonicalNameString());
+            Log.info(c, m, "Found pullable image: " + image.asCanonicalNameString());
             return Optional.of(cachedImage);
         } catch (Exception e) {
-            Log.info(c, m, "Unable to find pullable image " + image.asCanonicalNameString());
+            Log.info(c, m, "Unable to find pullable image: " + image.asCanonicalNameString());
             return Optional.empty();
         }
     }
@@ -130,11 +129,11 @@ private Optional<RemoteDockerImage> pullCached() {
      * Helper method, constructs an image from a Dockerfile
      */
     private RemoteDockerImage buildFromDockerfile() {
-        String resource = constructResource(image);
-        String baseImage = findBaseImageFrom(resource).asCanonicalNameString();
+        String resourcePath = constructResourcePath(image);
+        String baseImage = findBaseImageFrom(resourcePath).asCanonicalNameString();
 
         ImageFromDockerfile builtImage = new ImageFromDockerfile(image.asCanonicalNameString(), false)
-                        .withFileFromClasspath(".", resource)
+                        .withFileFromClasspath(".", resourcePath)
                         .withBuildArg(BASE_IMAGE, baseImage);
 
         return new RemoteDockerImage(builtImage);
@@ -145,10 +144,11 @@ private RemoteDockerImage buildFromDockerfile() {
      * Dockerfile and supporting files that define this image.
      *
      *
-     * @param  image The name of this image in the form: openliberty/testcontainers/<image-name>:<image-version>
+     * @param  image The name of this image in the form:
+     *                   [registry | localhost]/openliberty/testcontainers/<image-name>:<image-version>
      * @return       the resource path in the form: /openliberty/testcontainers/<image-name>/<image-version>/
      */
-    private static String constructResource(DockerImageName image) {
+    private static String constructResourcePath(DockerImageName image) {
         StringBuffer buffer = new StringBuffer();
         buffer.append("/");
         buffer.append(image.getRepository()).append("/");
@@ -158,26 +158,27 @@ private static String constructResource(DockerImageName image) {
     }
 
     /**
-     * Helper method, searches for the Dockerfile on the classpath,
-     * and attempts to find the line:
+     * Helper method, searches for the Dockerfile on the classpath
+     * given it's resource path, and attempts to find the line:
      * - ARG BASE_IMAGE="[base-image-of-docker-file]"
      *
      * Once found, run the BASE_IMAGE through the ImageNameSubstitutor
      * and return the DockerImageName result.
      *
-     * @param  resource the resource path of the Dockerfile
-     * @return          The substituted docker image of the BASE_IMAGE argument
+     * @param  resourcePath of the directory that contains a Dockerfile
+     * @return              The substituted docker image of the BASE_IMAGE argument
      */
-    private static DockerImageName findBaseImageFrom(String resource) {
+    private static DockerImageName findBaseImageFrom(String resourcePath) {
         final String BASE_IMAGE_PREFIX = "ARG BASE_IMAGE=\"";
 
         /*
-         * Finds the Dockerfile on classpath and will extract the file to a temporary location so we can read it.
+         * Finds the resource directory on the classpath and will extract the directory to a temporary location so we can read it.
          * This will be done during the image build step anyway so this is just front-loading that work for our benefit.
          */
-        String resourceDir = MountableFile.forClasspathResource(resource).getResolvedPath();
+        String resourceDir = MountableFile.forClasspathResource(resourcePath).getResolvedPath();
 
         Stream<String> dockerfileLines;
+
         try {
             dockerfileLines = Files.readAllLines(Paths.get(resourceDir, "Dockerfile")).stream();
         } catch (IOException e) {
@@ -193,30 +194,33 @@ private static DockerImageName findBaseImageFrom(String resource) {
 
         String baseImageName = baseImageLine.substring(BASE_IMAGE_PREFIX.length(), baseImageLine.lastIndexOf('"'));
 
+        // NOTE: this is NOT the ImageBuilderSubstitutor
         return ImageNameSubstitutor.instance().apply(DockerImageName.parse(baseImageName));
     }
 
     /**
-     * A ImageNameSubstitutor for images built by this outer class
+     * An ImageNameSubstitutor for images built by this outer class.
+     * Built images are not kept in a public registry and are typically cached locally
+     * or within an internal registry on the network.
      */
     private static class ImageBuilderSubstitutor extends ImageNameSubstitutor {
 
-        // TODO replace with the finalized property expected on our build systems
-        private static final String INTERNAL_REGISTRY_PROP = "io.openliberty.internal.registry";
+        private static final String INTERNAL_REGISTRY_PROP = "docker_registry.server";
 
         // Ensures when we look for cached images Docker only attempt to find images
         // locally or from an internally configured registry.
         private static final String REGISTRY = System.getProperty(INTERNAL_REGISTRY_PROP, "localhost");
 
-        // The repository where all Open Liberty images will be cached
+        // The repository where all Open Liberty images are located
         private static final String REPOSITORY_PREFIX = "openliberty/testcontainers/";
 
         @Override
         public DockerImageName apply(final DockerImageName original) {
             Objects.requireNonNull(original);
 
             if (!original.getRegistry().isEmpty()) {
-                throw new IllegalArgumentException("DockerImageName with the registry " + original.getRegistry() + " cannot be substituted with registry " + REGISTRY);
+                throw new IllegalArgumentException("DockerImageName with the registry " + original.getRegistry() +
+                                                   " cannot be substituted with registry " + REGISTRY);
             }
 
             if (original.getRepository().startsWith(REPOSITORY_PREFIX)) {
@@ -231,7 +235,7 @@ protected String getDescription() {
             return "ImageBuilderSubstitutor with registry " + REGISTRY;
         }
 
-        // Hide instance method from parent class.
+        // Hide instance method from parent class
         // which will choose the ImageNameSubstitutor based on environment.
         private static ImageBuilderSubstitutor instance;
 

dev/fattest.simplicity/test/componenttest/containers/ImageBuilderTest.java

@@ -39,6 +39,8 @@ public void imageBuilderSubstitutorTest() throws Exception {
             assertEquals("expected " + expected.asCanonicalNameString() + " but was " + actual.asCanonicalNameString(),
                          expected, actual);
         }
+
+        // TODO write test when using an internal registry
     }
 
     @Test

dev/io.openliberty.org.testcontainers/build.gradle

@@ -21,20 +21,22 @@ dependencies {
 }
 
 task assembleTestContainerData(type: JavaExec) {
-    onlyIf { // Do not execute this task during our builds
+    // Do not execute this task during our builds
+    onlyIf {
         !isAutomatedBuild
     }
 
-    // Use ArtifactoryMirrorSubstitutor to generate images file
+    // Use ArtifactoryMirrorSubstitutor to generate image names for cache/images
     environment "TESTCONTAINERS_IMAGE_SUBSTITUTOR", "componenttest.containers.ArtifactoryMirrorSubstitutor"
 
+    // Run main method of CacheFiles class
     classpath = configurations.runtime.plus(sourceSets.main.runtimeClasspath)
     main = "io.openliberty.org.testcontainers.generate.CacheFiles"
     args project.getProjectDir()
 
     // Debugging properties for testcontainers
     systemProperties = [
-      'org.slf4j.simpleLogger.logFile': new File(project.getProjectDir(), 'build/CacheFiles.log'),
+      'org.slf4j.simpleLogger.logFile': new File(project.getProjectDir(), 'build/cache.log'),
       'org.slf4j.simpleLogger.showDateTime': 'true',
       'org.slf4j.simpleLogger.dateTimeFormat': '[MM/dd/yyyy HH:mm:ss:SSS z]',
       'org.slf4j.simpleLogger.log.org.testcontainers': 'debug',
@@ -45,29 +47,30 @@ task assembleTestContainerData(type: JavaExec) {
 }
 
 task generateCustomImages(type: JavaExec) {
-    // If artifactory is available then use it for substitution
+    // If artifactory is available then use it for BASE_NAME substitution
     if ( gradle.userProps.containsKey("artifactory.docker.server") ) {
         environment "TESTCONTAINERS_IMAGE_SUBSTITUTOR", "componenttest.containers.ArtifactoryMirrorSubstitutor"
         environment "ARTIFACTORY_REGISTRY", gradle.userProps.getProperty("artifactory.docker.server")
     }
 
-    // If custom registry is available then use it for building
+    // If internal registry is available then use it for caching image
     if ( gradle.userProps.containsKey("docker_registry.server") ) {
-    	environment "INTERNAL_REGISTRY", gradle.userProps.getProperty("docker_registry.server")
+        environment "INTERNAL_REGISTRY", gradle.userProps.getProperty("docker_registry.server")
     }
 
+    // Run main method of CustomImages class
     classpath = configurations.runtime.plus(sourceSets.main.runtimeClasspath)
     main = "io.openliberty.org.testcontainers.generate.CustomImages"
     args project.getProjectDir()
 
     // Debugging properties for testcontainers
     systemProperties = [
-      'org.slf4j.simpleLogger.logFile': new File(project.getProjectDir(), 'build/CustomImages.log'),
+      'org.slf4j.simpleLogger.logFile': new File(project.getProjectDir(), 'build/build.log'),
       'org.slf4j.simpleLogger.showDateTime': 'true',
       'org.slf4j.simpleLogger.dateTimeFormat': '[MM/dd/yyyy HH:mm:ss:SSS z]',
       'org.slf4j.simpleLogger.log.org.testcontainers': 'debug',
       'org.slf4j.simpleLogger.log.tc': 'debug',
       'org.slf4j.simpleLogger.log.com.github.dockerjava': 'warn',
       'org.slf4j.simpleLogger.log.com.github.dockerjava.zerodep.shaded.org.apache.hc.client5.http.wire': 'off'
     ]
-}
+}

dev/io.openliberty.org.testcontainers/src/io/openliberty/org/testcontainers/generate/CustomImages.java

@@ -23,9 +23,8 @@
  */
 public class CustomImages {
 
-    // The --build-arg necessary to overwrite the default BASE_IMAGE in the
-    // Dockerfile
-    // with the mirrored image in artifactory
+    // The --build-arg necessary to overwrite the default BASE_IMAGE in the Dockerfile
+    // with the mirrored image from an alternative registry
     public static final String BASE_IMAGE = "BASE_IMAGE";
 
     public static void main(String[] args) {
@@ -42,7 +41,7 @@ public static void main(String[] args) {
         // Where to find instructions to build images
         Path commonPath = Paths.get(projectPath, "resources", "openliberty", "testcontainers");
 
-        // Construct a list of Dockerfiles
+        // Find all dockerfiles and attempt to build their corresponding images
         Dockerfile.findDockerfiles(commonPath).stream()
                 .map(location -> new Dockerfile(location))
                 .forEach(dockerfile -> {

dev/io.openliberty.org.testcontainers/src/io/openliberty/org/testcontainers/generate/Dockerfile.java

@@ -13,7 +13,7 @@
 import org.testcontainers.utility.ImageNameSubstitutor;
 
 /**
- * A record of a Dockerfile
+ * A record that represents a Dockerfile
  */
 public class Dockerfile {
     public final Path location;
@@ -136,10 +136,10 @@ private DockerImageName substituteBaseImage(final DockerImageName original) {
 
     /**
      * A ImageNameSubstitutor for images built by this outer class.
+     * TODO figure out if there is a way to use the ImageBuilderSubstitutor from the ImageBuilder class of fattest.simplicity
      */
     private static class ImageBuilderSubstitutor extends ImageNameSubstitutor {
 
-        // TODO replace with the finalized property expected on our build systems
         private static final String INTERNAL_REGISTRY_ENV = "INTERNAL_REGISTRY";
 
         // Ensures when we look for cached images Docker only attempt to find images
@@ -148,7 +148,7 @@ private static class ImageBuilderSubstitutor extends ImageNameSubstitutor {
                 ? "localhost"
                 : System.getenv(INTERNAL_REGISTRY_ENV);
 
-        // The repository where all Open Liberty images will be cached
+        // The repository where all Open Liberty images are located
         private static final String REPOSITORY_PREFIX = "openliberty/testcontainers/";
 
         @Override