From a454011b517434b24117bcf3f90b06dbff139617 Mon Sep 17 00:00:00 2001 From: Chanseok Oh Date: Fri, 8 Jan 2021 11:29:29 -0500 Subject: [PATCH 1/5] Remove unnecessary file lock --- .../jib/builder/steps/WriteTarFileStep.java | 3 +- .../tools/jib/cache/CacheStorageWriter.java | 3 +- .../tools/jib/filesystem/FileOperations.java | 33 ----------- .../jib/filesystem/FileOperationsTest.java | 58 +------------------ 4 files changed, 3 insertions(+), 94 deletions(-) diff --git a/jib-core/src/main/java/com/google/cloud/tools/jib/builder/steps/WriteTarFileStep.java b/jib-core/src/main/java/com/google/cloud/tools/jib/builder/steps/WriteTarFileStep.java index 88688a1375..f531917dad 100644 --- a/jib-core/src/main/java/com/google/cloud/tools/jib/builder/steps/WriteTarFileStep.java +++ b/jib-core/src/main/java/com/google/cloud/tools/jib/builder/steps/WriteTarFileStep.java @@ -19,7 +19,6 @@ import com.google.cloud.tools.jib.api.LogEvent; import com.google.cloud.tools.jib.builder.ProgressEventDispatcher; import com.google.cloud.tools.jib.configuration.BuildContext; -import com.google.cloud.tools.jib.filesystem.FileOperations; import com.google.cloud.tools.jib.image.Image; import com.google.cloud.tools.jib.image.ImageTarball; import java.io.BufferedOutputStream; @@ -59,7 +58,7 @@ public BuildResult call() throws IOException { Files.createDirectories(outputPath.getParent()); } try (OutputStream outputStream = - new BufferedOutputStream(FileOperations.newLockingOutputStream(outputPath))) { + new BufferedOutputStream(Files.newOutputStream(outputPath))) { new ImageTarball( builtImage, buildContext.getTargetImageConfiguration().getImage(), diff --git a/jib-core/src/main/java/com/google/cloud/tools/jib/cache/CacheStorageWriter.java b/jib-core/src/main/java/com/google/cloud/tools/jib/cache/CacheStorageWriter.java index 29d5758902..68bb886cfa 100644 --- a/jib-core/src/main/java/com/google/cloud/tools/jib/cache/CacheStorageWriter.java +++ b/jib-core/src/main/java/com/google/cloud/tools/jib/cache/CacheStorageWriter.java @@ -21,7 +21,6 @@ import com.google.cloud.tools.jib.blob.Blob; import com.google.cloud.tools.jib.blob.BlobDescriptor; import com.google.cloud.tools.jib.blob.Blobs; -import com.google.cloud.tools.jib.filesystem.FileOperations; import com.google.cloud.tools.jib.filesystem.LockFile; import com.google.cloud.tools.jib.filesystem.TempDirectoryProvider; import com.google.cloud.tools.jib.hash.CountingDigestOutputStream; @@ -447,7 +446,7 @@ private void writeSelector(DescriptorDigest selector, DescriptorDigest layerDige // Writes the selector to a temporary file and then moves the file to the intended location. Path temporarySelectorFile = Files.createTempFile(null, null); temporarySelectorFile.toFile().deleteOnExit(); - try (OutputStream fileOut = FileOperations.newLockingOutputStream(temporarySelectorFile)) { + try (OutputStream fileOut = Files.newOutputStream(temporarySelectorFile)) { fileOut.write(layerDigest.getHash().getBytes(StandardCharsets.UTF_8)); } diff --git a/jib-core/src/main/java/com/google/cloud/tools/jib/filesystem/FileOperations.java b/jib-core/src/main/java/com/google/cloud/tools/jib/filesystem/FileOperations.java index d4f6e09839..75f9df0917 100644 --- a/jib-core/src/main/java/com/google/cloud/tools/jib/filesystem/FileOperations.java +++ b/jib-core/src/main/java/com/google/cloud/tools/jib/filesystem/FileOperations.java @@ -18,15 +18,8 @@ import com.google.common.collect.ImmutableList; import java.io.IOException; -import java.io.OutputStream; -import java.nio.channels.Channels; -import java.nio.channels.FileChannel; -import java.nio.channels.FileLock; -import java.nio.channels.OverlappingFileLockException; import java.nio.file.Files; import java.nio.file.Path; -import java.nio.file.StandardOpenOption; -import java.util.EnumSet; /** Static methods for operating on the filesystem. */ public class FileOperations { @@ -59,31 +52,5 @@ public static void copy(ImmutableList sourceFiles, Path destDir) throws IO } } - /** - * Acquires an exclusive {@link FileLock} on the {@code file} and opens an {@link OutputStream} to - * write to it. The file will be created if it does not exist, or truncated to length 0 if it does - * exist. The {@link OutputStream} must be closed to release the lock. - * - *

The locking mechanism should not be used as a concurrency management feature. Rather, this - * should be used as a way to prevent concurrent writes to {@code file}. Concurrent attempts to - * lock {@code file} will result in {@link OverlappingFileLockException}s. - * - * @param file the file to write to - * @return an {@link OutputStream} that writes to the file - * @throws IOException if an I/O exception occurs - */ - public static OutputStream newLockingOutputStream(Path file) throws IOException { - EnumSet createOrTruncate = - EnumSet.of( - StandardOpenOption.CREATE, - StandardOpenOption.WRITE, - StandardOpenOption.TRUNCATE_EXISTING); - // Channel is closed by outputStream.close(). - FileChannel channel = FileChannel.open(file, createOrTruncate); - // Lock is released when channel is closed. - channel.lock(); - return Channels.newOutputStream(channel); - } - private FileOperations() {} } diff --git a/jib-core/src/test/java/com/google/cloud/tools/jib/filesystem/FileOperationsTest.java b/jib-core/src/test/java/com/google/cloud/tools/jib/filesystem/FileOperationsTest.java index 6ca3ee91a3..abfe2f3e4f 100644 --- a/jib-core/src/test/java/com/google/cloud/tools/jib/filesystem/FileOperationsTest.java +++ b/jib-core/src/test/java/com/google/cloud/tools/jib/filesystem/FileOperationsTest.java @@ -17,22 +17,12 @@ package com.google.cloud.tools.jib.filesystem; import com.google.common.collect.ImmutableList; -import com.google.common.io.CharStreams; import com.google.common.io.Resources; import java.io.IOException; -import java.io.InputStream; -import java.io.InputStreamReader; -import java.io.OutputStream; import java.net.URISyntaxException; -import java.nio.channels.Channels; -import java.nio.channels.FileChannel; -import java.nio.channels.FileLock; -import java.nio.channels.OverlappingFileLockException; -import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; -import java.nio.file.StandardOpenOption; import org.junit.Assert; import org.junit.Rule; import org.junit.Test; @@ -41,32 +31,7 @@ /** Tests for {@link FileOperations}. */ public class FileOperationsTest { - private static void verifyWriteWithLock(Path file) throws IOException { - OutputStream fileOutputStream = FileOperations.newLockingOutputStream(file); - - // Checks that the file was locked. - try (FileChannel channel = FileChannel.open(file, StandardOpenOption.READ)) { - // locking should either fail and return null or throw an OverlappingFileLockException - FileLock lock = channel.tryLock(0, Long.MAX_VALUE, true); - Assert.assertNull("Lock attempt should have failed", lock); - - } catch (OverlappingFileLockException ex) { - // pass - } - - fileOutputStream.write("jib".getBytes(StandardCharsets.UTF_8)); - fileOutputStream.close(); - - FileChannel channel = FileChannel.open(file, StandardOpenOption.READ); - channel.lock(0, Long.MAX_VALUE, true); - try (InputStream inputStream = Channels.newInputStream(channel); - InputStreamReader inputStreamReader = - new InputStreamReader(inputStream, StandardCharsets.UTF_8)) { - Assert.assertEquals("jib", CharStreams.toString(inputStreamReader)); - } - } - - @Rule public TemporaryFolder temporaryFolder = new TemporaryFolder(); + @Rule public final TemporaryFolder temporaryFolder = new TemporaryFolder(); @Test public void testCopy() throws IOException, URISyntaxException { @@ -91,27 +56,6 @@ public void testCopy() throws IOException, URISyntaxException { assertFilesEqual(dirLayer.resolve("foo"), destDir.resolve("layer").resolve("foo")); } - @Test - public void testNewLockingOutputStream_newFile() throws IOException { - Path file = Files.createTempFile("", ""); - // Ensures file doesn't exist. - Assert.assertTrue(Files.deleteIfExists(file)); - - verifyWriteWithLock(file); - } - - @Test - public void testNewLockingOutputStream_existingFile() throws IOException { - Path file = Files.createTempFile("", ""); - // Writes out more bytes to ensure proper truncated. - byte[] dataBytes = new byte[] {0, 1, 2, 3, 4, 5, 6, 7, 8, 9}; - Files.write(file, dataBytes, StandardOpenOption.WRITE); - Assert.assertTrue(Files.exists(file)); - Assert.assertEquals(10, Files.size(file)); - - verifyWriteWithLock(file); - } - private void assertFilesEqual(Path file1, Path file2) throws IOException { Assert.assertArrayEquals(Files.readAllBytes(file1), Files.readAllBytes(file2)); } From 3412f5446b4bd22d2eee95407c2f194db18723f4 Mon Sep 17 00:00:00 2001 From: Chanseok Oh Date: Tue, 12 Jan 2021 16:14:46 -0500 Subject: [PATCH 2/5] wip --- .../tools/jib/gradle/BuildDockerTask.java | 3 +- .../tools/jib/gradle/BuildImageTask.java | 3 +- .../cloud/tools/jib/gradle/BuildTarTask.java | 3 +- .../cloud/tools/jib/gradle/TaskCommon.java | 11 +- .../tools/jib/maven/BuildDockerMojo.java | 11 +- .../cloud/tools/jib/maven/BuildImageMojo.java | 11 +- .../cloud/tools/jib/maven/BuildTarMojo.java | 11 +- .../cloud/tools/jib/maven/MojoCommon.java | 14 +- jib-plugins-common/build.gradle | 3 + .../jib/plugins/common/UpdateChecker.java | 124 +++--------------- .../common/globalconfig/GlobalConfig.java | 98 ++++++++++++++ .../globalconfig/GlobalConfigTemplate.java | 19 +++ .../jib/plugins/common/UpdateCheckerTest.java | 83 ++++-------- .../common/globalconfig/GlobalConfigTest.java | 79 +++++++++++ 14 files changed, 291 insertions(+), 182 deletions(-) create mode 100644 jib-plugins-common/src/main/java/com/google/cloud/tools/jib/plugins/common/globalconfig/GlobalConfig.java create mode 100644 jib-plugins-common/src/main/java/com/google/cloud/tools/jib/plugins/common/globalconfig/GlobalConfigTemplate.java create mode 100644 jib-plugins-common/src/test/java/com/google/cloud/tools/jib/plugins/common/globalconfig/GlobalConfigTest.java diff --git a/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildDockerTask.java b/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildDockerTask.java index 25fd79e2ff..ca5706ce9f 100644 --- a/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildDockerTask.java +++ b/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildDockerTask.java @@ -32,6 +32,7 @@ import com.google.cloud.tools.jib.plugins.common.InvalidWorkingDirectoryException; import com.google.cloud.tools.jib.plugins.common.MainClassInferenceException; import com.google.cloud.tools.jib.plugins.common.PluginConfigurationProcessor; +import com.google.cloud.tools.jib.plugins.common.globalconfig.GlobalConfig; import com.google.cloud.tools.jib.plugins.extension.JibPluginExtensionException; import com.google.common.base.Preconditions; import java.io.IOException; @@ -105,7 +106,7 @@ public void buildDocker() GradleProjectProperties projectProperties = GradleProjectProperties.getForProject(getProject(), getLogger(), tempDirectoryProvider); Future> updateCheckFuture = - TaskCommon.newUpdateChecker(projectProperties, getLogger()); + TaskCommon.newUpdateChecker(projectProperties, GlobalConfig.readConfig(), getLogger()); try { PluginConfigurationProcessor.createJibBuildRunnerForDockerDaemonImage( new GradleRawConfiguration(jibExtension), diff --git a/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildImageTask.java b/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildImageTask.java index 86f429fac2..2e9ac562a8 100644 --- a/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildImageTask.java +++ b/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildImageTask.java @@ -31,6 +31,7 @@ import com.google.cloud.tools.jib.plugins.common.InvalidWorkingDirectoryException; import com.google.cloud.tools.jib.plugins.common.MainClassInferenceException; import com.google.cloud.tools.jib.plugins.common.PluginConfigurationProcessor; +import com.google.cloud.tools.jib.plugins.common.globalconfig.GlobalConfig; import com.google.cloud.tools.jib.plugins.extension.JibPluginExtensionException; import com.google.common.base.Preconditions; import com.google.common.base.Strings; @@ -93,7 +94,7 @@ public void buildImage() GradleProjectProperties projectProperties = GradleProjectProperties.getForProject(getProject(), getLogger(), tempDirectoryProvider); Future> updateCheckFuture = - TaskCommon.newUpdateChecker(projectProperties, getLogger()); + TaskCommon.newUpdateChecker(projectProperties, GlobalConfig.readConfig(), getLogger()); try { if (Strings.isNullOrEmpty(jibExtension.getTo().getImage())) { throw new GradleException( diff --git a/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildTarTask.java b/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildTarTask.java index 5f1d95a6ec..679a821b09 100644 --- a/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildTarTask.java +++ b/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildTarTask.java @@ -31,6 +31,7 @@ import com.google.cloud.tools.jib.plugins.common.InvalidWorkingDirectoryException; import com.google.cloud.tools.jib.plugins.common.MainClassInferenceException; import com.google.cloud.tools.jib.plugins.common.PluginConfigurationProcessor; +import com.google.cloud.tools.jib.plugins.common.globalconfig.GlobalConfig; import com.google.cloud.tools.jib.plugins.extension.JibPluginExtensionException; import com.google.common.base.Preconditions; import java.io.IOException; @@ -126,7 +127,7 @@ public void buildTar() GradleProjectProperties projectProperties = GradleProjectProperties.getForProject(getProject(), getLogger(), tempDirectoryProvider); Future> updateCheckFuture = - TaskCommon.newUpdateChecker(projectProperties, getLogger()); + TaskCommon.newUpdateChecker(projectProperties, GlobalConfig.readConfig(), getLogger()); try { PluginConfigurationProcessor.createJibBuildRunnerForTarImage( new GradleRawConfiguration(jibExtension), diff --git a/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/TaskCommon.java b/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/TaskCommon.java index 0149c49d1d..3588048947 100644 --- a/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/TaskCommon.java +++ b/jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/TaskCommon.java @@ -22,6 +22,7 @@ import com.google.cloud.tools.jib.api.buildplan.FilePermissions; import com.google.cloud.tools.jib.plugins.common.ProjectProperties; import com.google.cloud.tools.jib.plugins.common.UpdateChecker; +import com.google.cloud.tools.jib.plugins.common.globalconfig.GlobalConfig; import com.google.common.util.concurrent.Futures; import java.util.LinkedHashMap; import java.util.Map; @@ -47,18 +48,20 @@ class TaskCommon { public static final String VERSION_URL = "https://storage.googleapis.com/jib-versions/jib-gradle"; static Future> newUpdateChecker( - ProjectProperties projectProperties, Logger logger) { - if (projectProperties.isOffline() || !logger.isLifecycleEnabled()) { + ProjectProperties projectProperties, GlobalConfig globalConfig, Logger logger) { + if (projectProperties.isOffline() + || !logger.isLifecycleEnabled() + || globalConfig.isDisableUpdateCheck()) { return Futures.immediateFuture(Optional.empty()); } ExecutorService executorService = Executors.newSingleThreadExecutor(); try { return UpdateChecker.checkForUpdate( executorService, - projectProperties::log, VERSION_URL, projectProperties.getToolName(), - projectProperties.getToolVersion()); + projectProperties.getToolVersion(), + projectProperties::log); } finally { executorService.shutdown(); } diff --git a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildDockerMojo.java b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildDockerMojo.java index e75e57ab7b..dec33eeb97 100644 --- a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildDockerMojo.java +++ b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildDockerMojo.java @@ -32,6 +32,7 @@ import com.google.cloud.tools.jib.plugins.common.InvalidWorkingDirectoryException; import com.google.cloud.tools.jib.plugins.common.MainClassInferenceException; import com.google.cloud.tools.jib.plugins.common.PluginConfigurationProcessor; +import com.google.cloud.tools.jib.plugins.common.globalconfig.GlobalConfig; import com.google.cloud.tools.jib.plugins.extension.JibPluginExtensionException; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; @@ -82,8 +83,16 @@ public void execute() throws MojoExecutionException, MojoFailureException { getSession(), getLog(), tempDirectoryProvider); + + GlobalConfig globalConfig = null; + try { + globalConfig = GlobalConfig.readConfig(); + } catch (IOException ex) { + throw new MojoExecutionException(ex.getMessage(), ex); + } Future> updateCheckFuture = - MojoCommon.newUpdateChecker(projectProperties, getLog()); + MojoCommon.newUpdateChecker(projectProperties, globalConfig, getLog()); + try { PluginConfigurationProcessor.createJibBuildRunnerForDockerDaemonImage( new MavenRawConfiguration(this), diff --git a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildImageMojo.java b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildImageMojo.java index 53530cf76b..f105713459 100644 --- a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildImageMojo.java +++ b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildImageMojo.java @@ -32,6 +32,7 @@ import com.google.cloud.tools.jib.plugins.common.InvalidWorkingDirectoryException; import com.google.cloud.tools.jib.plugins.common.MainClassInferenceException; import com.google.cloud.tools.jib.plugins.common.PluginConfigurationProcessor; +import com.google.cloud.tools.jib.plugins.common.globalconfig.GlobalConfig; import com.google.cloud.tools.jib.plugins.extension.JibPluginExtensionException; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; @@ -96,8 +97,16 @@ public void execute() throws MojoExecutionException, MojoFailureException { getSession(), getLog(), tempDirectoryProvider); + + GlobalConfig globalConfig = null; + try { + globalConfig = GlobalConfig.readConfig(); + } catch (IOException ex) { + throw new MojoExecutionException(ex.getMessage(), ex); + } Future> updateCheckFuture = - MojoCommon.newUpdateChecker(projectProperties, getLog()); + MojoCommon.newUpdateChecker(projectProperties, globalConfig, getLog()); + try { PluginConfigurationProcessor.createJibBuildRunnerForRegistryImage( new MavenRawConfiguration(this), diff --git a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildTarMojo.java b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildTarMojo.java index b514ffa7a1..959dc2fd07 100644 --- a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildTarMojo.java +++ b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildTarMojo.java @@ -31,6 +31,7 @@ import com.google.cloud.tools.jib.plugins.common.InvalidWorkingDirectoryException; import com.google.cloud.tools.jib.plugins.common.MainClassInferenceException; import com.google.cloud.tools.jib.plugins.common.PluginConfigurationProcessor; +import com.google.cloud.tools.jib.plugins.common.globalconfig.GlobalConfig; import com.google.cloud.tools.jib.plugins.extension.JibPluginExtensionException; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; @@ -74,8 +75,16 @@ public void execute() throws MojoExecutionException, MojoFailureException { getSession(), getLog(), tempDirectoryProvider); + + GlobalConfig globalConfig = null; + try { + globalConfig = GlobalConfig.readConfig(); + } catch (IOException ex) { + throw new MojoExecutionException(ex.getMessage(), ex); + } Future> updateCheckFuture = - MojoCommon.newUpdateChecker(projectProperties, getLog()); + MojoCommon.newUpdateChecker(projectProperties, globalConfig, getLog()); + try { PluginConfigurationProcessor.createJibBuildRunnerForTarImage( new MavenRawConfiguration(this), diff --git a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/MojoCommon.java b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/MojoCommon.java index 6975d13578..655d657d2c 100644 --- a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/MojoCommon.java +++ b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/MojoCommon.java @@ -16,16 +16,16 @@ package com.google.cloud.tools.jib.maven; -import static com.google.cloud.tools.jib.maven.JibPluginConfiguration.ExtraDirectoryParameters; - import com.google.cloud.tools.jib.ProjectInfo; import com.google.cloud.tools.jib.api.LogEvent; import com.google.cloud.tools.jib.api.buildplan.FilePermissions; +import com.google.cloud.tools.jib.maven.JibPluginConfiguration.ExtraDirectoryParameters; import com.google.cloud.tools.jib.maven.JibPluginConfiguration.PermissionConfiguration; import com.google.cloud.tools.jib.plugins.common.ProjectProperties; import com.google.cloud.tools.jib.plugins.common.PropertyNames; import com.google.cloud.tools.jib.plugins.common.UpdateChecker; import com.google.cloud.tools.jib.plugins.common.VersionChecker; +import com.google.cloud.tools.jib.plugins.common.globalconfig.GlobalConfig; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.util.concurrent.Futures; @@ -53,18 +53,20 @@ public class MojoCommon { public static final String VERSION_URL = "https://storage.googleapis.com/jib-versions/jib-maven"; static Future> newUpdateChecker( - ProjectProperties projectProperties, Log logger) { - if (projectProperties.isOffline() || !logger.isInfoEnabled()) { + ProjectProperties projectProperties, GlobalConfig globalConfig, Log logger) { + if (projectProperties.isOffline() + || !logger.isInfoEnabled() + || globalConfig.isDisableUpdateCheck()) { return Futures.immediateFuture(Optional.empty()); } ExecutorService executorService = Executors.newSingleThreadExecutor(); try { return UpdateChecker.checkForUpdate( executorService, - projectProperties::log, VERSION_URL, projectProperties.getToolName(), - projectProperties.getToolVersion()); + projectProperties.getToolVersion(), + projectProperties::log); } finally { executorService.shutdown(); } diff --git a/jib-plugins-common/build.gradle b/jib-plugins-common/build.gradle index 4459d8aec1..6adf215884 100644 --- a/jib-plugins-common/build.gradle +++ b/jib-plugins-common/build.gradle @@ -14,6 +14,9 @@ dependencies { implementation dependencyStrings.EXTENSION_COMMON testImplementation dependencyStrings.JUNIT + testImplementation dependencyStrings.JUNIT_PARAMS + testImplementation dependencyStrings.TRUTH + testImplementation dependencyStrings.TRUTH8 testImplementation dependencyStrings.MOCKITO_CORE testImplementation dependencyStrings.SLF4J_API testImplementation dependencyStrings.SYSTEM_RULES diff --git a/jib-plugins-common/src/main/java/com/google/cloud/tools/jib/plugins/common/UpdateChecker.java b/jib-plugins-common/src/main/java/com/google/cloud/tools/jib/plugins/common/UpdateChecker.java index 291b2ecc67..2ff32defb0 100644 --- a/jib-plugins-common/src/main/java/com/google/cloud/tools/jib/plugins/common/UpdateChecker.java +++ b/jib-plugins-common/src/main/java/com/google/cloud/tools/jib/plugins/common/UpdateChecker.java @@ -18,22 +18,18 @@ import com.fasterxml.jackson.annotation.JsonIgnoreProperties; import com.google.cloud.tools.jib.api.LogEvent; -import com.google.cloud.tools.jib.filesystem.XdgDirectories; import com.google.cloud.tools.jib.http.FailoverHttpClient; import com.google.cloud.tools.jib.http.Request; import com.google.cloud.tools.jib.http.Response; import com.google.cloud.tools.jib.json.JsonTemplate; import com.google.cloud.tools.jib.json.JsonTemplateMapper; +import com.google.cloud.tools.jib.plugins.common.globalconfig.GlobalConfig; import com.google.common.annotations.VisibleForTesting; -import com.google.common.base.Strings; import java.io.IOException; -import java.io.OutputStream; import java.net.URL; import java.nio.charset.StandardCharsets; -import java.nio.file.AtomicMoveNotSupportedException; import java.nio.file.Files; import java.nio.file.Path; -import java.nio.file.Paths; import java.nio.file.StandardCopyOption; import java.time.Duration; import java.time.Instant; @@ -47,20 +43,8 @@ /** Checks if Jib is up-to-date. */ public class UpdateChecker { - private static final String CONFIG_FILENAME = "config.json"; private static final String LAST_UPDATE_CHECK_FILENAME = "lastUpdateCheck"; - /** JSON template for the configuration file used to enable/disable update checks. */ - @VisibleForTesting - static class ConfigJsonTemplate implements JsonTemplate { - private boolean disableUpdateCheck; - - @VisibleForTesting - void setDisableUpdateCheck(boolean disableUpdateCheck) { - this.disableUpdateCheck = disableUpdateCheck; - } - } - /** JSON template for content downloaded during version check. */ @JsonIgnoreProperties(ignoreUnknown = true) private static class VersionJsonTemplate implements JsonTemplate { @@ -71,79 +55,33 @@ private static class VersionJsonTemplate implements JsonTemplate { * Begins checking for an update in a separate thread. * * @param executorService the {@link ExecutorService} - * @param log {@link Consumer} used to log messages * @param versionUrl the location to check for the latest version * @param toolName the tool name * @param toolVersion the tool version + * @param log {@link Consumer} used to log messages * @return a new {@link UpdateChecker} */ public static Future> checkForUpdate( ExecutorService executorService, - Consumer log, String versionUrl, String toolName, - String toolVersion) { + String toolVersion, + Consumer log) { return executorService.submit( - () -> performUpdateCheck(log, toolVersion, versionUrl, getConfigDir(), toolName)); + () -> + performUpdateCheck( + GlobalConfig.getConfigDir(), toolVersion, versionUrl, toolName, log)); } @VisibleForTesting static Optional performUpdateCheck( - Consumer log, + Path configDir, String currentVersion, String versionUrl, - Path configDir, - String toolName) { - // Abort if offline or update checks are disabled - if (Boolean.getBoolean(PropertyNames.DISABLE_UPDATE_CHECKS)) { - return Optional.empty(); - } - - Path configFile = configDir.resolve(CONFIG_FILENAME); - Path lastUpdateCheck = configDir.resolve(LAST_UPDATE_CHECK_FILENAME); - + String toolName, + Consumer log) { try { - // Check global config - if (Files.exists(configFile) && Files.size(configFile) > 0) { - // Abort if update checks are disabled - try { - ConfigJsonTemplate config = - JsonTemplateMapper.readJsonFromFile(configFile, ConfigJsonTemplate.class); - if (config.disableUpdateCheck) { - return Optional.empty(); - } - } catch (IOException ex) { - log.accept( - LogEvent.warn( - "Failed to read global Jib config; you may need to fix or delete " - + configFile - + ": " - + ex.getMessage())); - return Optional.empty(); - } - } else { - // Generate config file if it doesn't exist - ConfigJsonTemplate config = new ConfigJsonTemplate(); - Files.createDirectories(configDir); - Path tempConfigFile = configDir.resolve(CONFIG_FILENAME + ".tmp"); - try (OutputStream outputStream = Files.newOutputStream(tempConfigFile)) { - JsonTemplateMapper.writeTo(config, outputStream); - tryAtomicMove(tempConfigFile, configFile); - } catch (IOException ex) { - // If attempt to generate new config file failed, delete so we can try again next time - log.accept(LogEvent.debug("Failed to generate global Jib config; " + ex.getMessage())); - try { - Files.deleteIfExists(tempConfigFile); - } catch (IOException cleanupEx) { - log.accept( - LogEvent.debug( - "Failed to cleanup " - + tempConfigFile.toString() - + " -- " - + cleanupEx.getMessage())); - } - } - } + Path lastUpdateCheck = configDir.resolve(LAST_UPDATE_CHECK_FILENAME); // Check time of last update check if (Files.exists(lastUpdateCheck)) { @@ -173,9 +111,13 @@ static Optional performUpdateCheck( .build()); VersionJsonTemplate version = JsonTemplateMapper.readJson(response.getBody(), VersionJsonTemplate.class); - Path lastUpdateCheckTemp = configDir.resolve(LAST_UPDATE_CHECK_FILENAME + ".tmp"); + + Path lastUpdateCheckTemp = + Files.createTempFile(configDir, LAST_UPDATE_CHECK_FILENAME, null); + lastUpdateCheckTemp.toFile().deleteOnExit(); Files.write(lastUpdateCheckTemp, Instant.now().toString().getBytes(StandardCharsets.UTF_8)); - tryAtomicMove(lastUpdateCheckTemp, lastUpdateCheck); + Files.move(lastUpdateCheckTemp, lastUpdateCheck, StandardCopyOption.REPLACE_EXISTING); + if (currentVersion.equals(version.latest)) { return Optional.empty(); } @@ -218,37 +160,5 @@ public static Optional finishUpdateCheck(Future> update return Optional.empty(); } - /** - * Returns the config directory set by {@link PropertyNames#CONFIG_DIRECTORY} if not null, - * otherwise returns the default config directory. - * - * @return the config directory set by {@link PropertyNames#CONFIG_DIRECTORY} if not null, - * otherwise returns the default config directory. - */ - private static Path getConfigDir() { - String configDirProperty = System.getProperty(PropertyNames.CONFIG_DIRECTORY); - if (!Strings.isNullOrEmpty(configDirProperty)) { - return Paths.get(configDirProperty); - } - return XdgDirectories.getConfigHome(); - } - - /** - * Attempts an atomic move first, and falls back to non-atomic if the file system does not support - * atomic moves. - * - * @param source the source path - * @param destination the destination path - * @throws IOException if the move fails - */ - private static void tryAtomicMove(Path source, Path destination) throws IOException { - try { - Files.move( - source, destination, StandardCopyOption.ATOMIC_MOVE, StandardCopyOption.REPLACE_EXISTING); - } catch (AtomicMoveNotSupportedException ignored) { - Files.move(source, destination, StandardCopyOption.REPLACE_EXISTING); - } - } - private UpdateChecker() {} } diff --git a/jib-plugins-common/src/main/java/com/google/cloud/tools/jib/plugins/common/globalconfig/GlobalConfig.java b/jib-plugins-common/src/main/java/com/google/cloud/tools/jib/plugins/common/globalconfig/GlobalConfig.java new file mode 100644 index 0000000000..9fec7cc44b --- /dev/null +++ b/jib-plugins-common/src/main/java/com/google/cloud/tools/jib/plugins/common/globalconfig/GlobalConfig.java @@ -0,0 +1,98 @@ +/* + * Copyright 2021 Google LLC. + * + * 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.plugins.common.globalconfig; + +import com.google.cloud.tools.jib.filesystem.XdgDirectories; +import com.google.cloud.tools.jib.json.JsonTemplateMapper; +import com.google.cloud.tools.jib.plugins.common.PropertyNames; +import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Strings; +import java.io.IOException; +import java.io.OutputStream; +import java.nio.file.FileAlreadyExistsException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; + +/** Represents read-only Jib global configuration. */ +public class GlobalConfig { + + private static final String CONFIG_FILENAME = "config.json"; + + public static GlobalConfig readConfig() throws IOException { + return readConfig(getConfigDir()); + } + + @VisibleForTesting + static GlobalConfig readConfig(Path configDir) throws IOException { + Path configFile = configDir.resolve(CONFIG_FILENAME); + + try { + if (Files.exists(configFile)) { + GlobalConfigTemplate configJson = + JsonTemplateMapper.readJsonFromFile(configFile, GlobalConfigTemplate.class); + return new GlobalConfig(configJson); + } + + // Generate config file if it doesn't exist + Files.createDirectories(configDir); + Path tempConfigFile = Files.createTempFile(configDir, CONFIG_FILENAME, null); + tempConfigFile.toFile().deleteOnExit(); + + GlobalConfigTemplate configJson = new GlobalConfigTemplate(); + try (OutputStream outputStream = Files.newOutputStream(tempConfigFile)) { + JsonTemplateMapper.writeTo(configJson, outputStream); + Files.move(tempConfigFile, configFile); + return new GlobalConfig(configJson); + + } catch (FileAlreadyExistsException ex) { + // Perhaps created concurrently. Read again. + return readConfig(configDir); + } + + } catch (IOException ex) { + throw new IOException( + "Failed to read global Jib config; you may need to fix or delete " + configFile, ex); + } + } + + /** + * Returns the config directory set by {@link PropertyNames#CONFIG_DIRECTORY} if not null, + * otherwise returns the default config directory. + * + * @return the config directory set by {@link PropertyNames#CONFIG_DIRECTORY} if not null, + * otherwise returns the default config directory. + */ + public static Path getConfigDir() { + String configDirProperty = System.getProperty(PropertyNames.CONFIG_DIRECTORY); + if (!Strings.isNullOrEmpty(configDirProperty)) { + return Paths.get(configDirProperty); + } + return XdgDirectories.getConfigHome(); + } + + private GlobalConfigTemplate jsonConfig; + + private GlobalConfig(GlobalConfigTemplate jsonConfig) { + this.jsonConfig = jsonConfig; + } + + public boolean isDisableUpdateCheck() { + return Boolean.getBoolean(PropertyNames.DISABLE_UPDATE_CHECKS) + || jsonConfig.isDisableUpdateCheck(); + } +} diff --git a/jib-plugins-common/src/main/java/com/google/cloud/tools/jib/plugins/common/globalconfig/GlobalConfigTemplate.java b/jib-plugins-common/src/main/java/com/google/cloud/tools/jib/plugins/common/globalconfig/GlobalConfigTemplate.java new file mode 100644 index 0000000000..424660dc9e --- /dev/null +++ b/jib-plugins-common/src/main/java/com/google/cloud/tools/jib/plugins/common/globalconfig/GlobalConfigTemplate.java @@ -0,0 +1,19 @@ +package com.google.cloud.tools.jib.plugins.common.globalconfig; + +import com.fasterxml.jackson.annotation.JsonIgnoreProperties; +import com.google.cloud.tools.jib.json.JsonTemplate; + +/** JSON template for the global configuration file. */ +@JsonIgnoreProperties(ignoreUnknown = true) +public class GlobalConfigTemplate implements JsonTemplate { + + private boolean disableUpdateCheck; + + public void setDisableUpdateCheck(boolean disableUpdateCheck) { + this.disableUpdateCheck = disableUpdateCheck; + } + + public boolean isDisableUpdateCheck() { + return disableUpdateCheck; + } +} diff --git a/jib-plugins-common/src/test/java/com/google/cloud/tools/jib/plugins/common/UpdateCheckerTest.java b/jib-plugins-common/src/test/java/com/google/cloud/tools/jib/plugins/common/UpdateCheckerTest.java index be9b14d7fa..17d14ef9a9 100644 --- a/jib-plugins-common/src/test/java/com/google/cloud/tools/jib/plugins/common/UpdateCheckerTest.java +++ b/jib-plugins-common/src/test/java/com/google/cloud/tools/jib/plugins/common/UpdateCheckerTest.java @@ -53,6 +53,7 @@ public class UpdateCheckerTest { private TestWebServer testWebServer; private Path configDir; + private String versionUrl; @Before public void setUp() @@ -64,6 +65,7 @@ public void setUp() "HTTP/1.1 200 OK\nContent-Length:18\n\n{\"latest\":\"2.0.0\"}"), 1); configDir = temporaryFolder.getRoot().toPath(); + versionUrl = testWebServer.getEndpoint(); } @After @@ -74,10 +76,10 @@ public void tearDown() throws IOException { @Test public void testPerformUpdateCheck_newVersionFound() throws IOException { Instant before = Instant.now(); - setupConfigAndLastUpdateCheck(); + setupLastUpdateCheck(); Optional message = UpdateChecker.performUpdateCheck( - ignored -> {}, "1.0.2", testWebServer.getEndpoint(), configDir, "tool name"); + configDir, "1.0.2", versionUrl, "tool name", ignored -> {}); Assert.assertTrue(testWebServer.getInputRead().contains("User-Agent: jib 1.0.2 tool name")); Assert.assertTrue(message.isPresent()); Assert.assertEquals( @@ -93,16 +95,13 @@ public void testPerformUpdateCheck_newVersionFound() throws IOException { @Test public void testPerformUpdateCheck_newJsonField() throws IOException, InterruptedException, GeneralSecurityException, URISyntaxException { - try (TestWebServer server = - new TestWebServer( - false, - Collections.singletonList( - "HTTP/1.1 200 OK\nContent-Length:43\n\n{\"latest\":\"2.0.0\",\"unknownField\":\"unknown\"}"), - 1)) { - setupConfigAndLastUpdateCheck(); + String response = + "HTTP/1.1 200 OK\nContent-Length:43\n\n{\"latest\":\"2.0.0\",\"unknownField\":\"unknown\"}"; + try (TestWebServer server = new TestWebServer(false, Collections.singletonList(response), 1)) { + setupLastUpdateCheck(); Optional message = UpdateChecker.performUpdateCheck( - ignored -> {}, "1.0.2", server.getEndpoint(), configDir, "tool name"); + configDir, "1.0.2", server.getEndpoint(), "tool name", ignored -> {}); Assert.assertTrue(message.isPresent()); Assert.assertEquals( "A new version of Jib (2.0.0) is available (currently using 1.0.2). Update your build " @@ -114,10 +113,10 @@ public void testPerformUpdateCheck_newJsonField() @Test public void testPerformUpdateCheck_onLatest() throws IOException { Instant before = Instant.now(); - setupConfigAndLastUpdateCheck(); + setupLastUpdateCheck(); Optional message = UpdateChecker.performUpdateCheck( - ignored -> {}, "2.0.0", testWebServer.getEndpoint(), configDir, "tool name"); + configDir, "2.0.0", versionUrl, "tool name", ignored -> {}); Assert.assertFalse(message.isPresent()); String modifiedTime = new String( @@ -127,11 +126,11 @@ public void testPerformUpdateCheck_onLatest() throws IOException { } @Test - public void testPerformUpdateCheck_noConfigOrLastUpdateCheck() throws IOException { + public void testPerformUpdateCheck_noLastUpdateCheck() throws IOException { Instant before = Instant.now(); Optional message = UpdateChecker.performUpdateCheck( - ignored -> {}, "1.0.2", testWebServer.getEndpoint(), configDir, "tool name"); + configDir, "1.0.2", versionUrl, "tool name", ignored -> {}); Assert.assertTrue(message.isPresent()); Assert.assertEquals( "A new version of Jib (2.0.0) is available (currently using 1.0.2). Update your build " @@ -144,13 +143,12 @@ public void testPerformUpdateCheck_noConfigOrLastUpdateCheck() throws IOExceptio } @Test - public void testPerformUpdateCheck_emptyConfigAndLastUpdateCheck() throws IOException { - Files.createFile(configDir.resolve("config.json")); + public void testPerformUpdateCheck_emptyLastUpdateCheck() throws IOException { Files.createFile(configDir.resolve("lastUpdateCheck")); Instant before = Instant.now(); Optional message = UpdateChecker.performUpdateCheck( - ignored -> {}, "1.0.2", testWebServer.getEndpoint(), configDir, "tool name"); + configDir, "1.0.2", versionUrl, "tool name", ignored -> {}); Assert.assertTrue(message.isPresent()); Assert.assertEquals( "A new version of Jib (2.0.0) is available (currently using 1.0.2). Update your build " @@ -165,13 +163,13 @@ public void testPerformUpdateCheck_emptyConfigAndLastUpdateCheck() throws IOExce @Test public void testPerformUpdateCheck_lastUpdateCheckTooSoon() throws IOException { FileTime modifiedTime = FileTime.from(Instant.now().minusSeconds(12)); - setupConfigAndLastUpdateCheck(); + setupLastUpdateCheck(); Files.write( configDir.resolve("lastUpdateCheck"), modifiedTime.toString().getBytes(StandardCharsets.UTF_8)); Optional message = UpdateChecker.performUpdateCheck( - ignored -> {}, "1.0.2", testWebServer.getEndpoint(), configDir, "tool name"); + configDir, "1.0.2", versionUrl, "tool name", ignored -> {}); Assert.assertFalse(message.isPresent()); // lastUpdateCheck should not have changed @@ -181,36 +179,6 @@ public void testPerformUpdateCheck_lastUpdateCheckTooSoon() throws IOException { Assert.assertEquals(Instant.parse(lastUpdateTime), modifiedTime.toInstant()); } - @Test - public void testPerformUpdateCheck_systemProperty() { - System.setProperty(PropertyNames.DISABLE_UPDATE_CHECKS, "true"); - Optional message = - UpdateChecker.performUpdateCheck( - ignored -> {}, "1.0.2", testWebServer.getEndpoint(), configDir, "tool name"); - Assert.assertFalse(message.isPresent()); - } - - @Test - public void testPerformUpdateCheck_configDisabled() throws IOException { - Files.write( - configDir.resolve("config.json"), - "{\"disableUpdateCheck\":true}".getBytes(StandardCharsets.UTF_8)); - Optional message = - UpdateChecker.performUpdateCheck( - ignored -> {}, "1.0.2", testWebServer.getEndpoint(), configDir, "tool name"); - Assert.assertFalse(message.isPresent()); - } - - @Test - public void testPerformUpdateCheck_badConfig() throws IOException { - Files.write( - configDir.resolve("config.json"), "corrupt config".getBytes(StandardCharsets.UTF_8)); - Optional message = - UpdateChecker.performUpdateCheck( - ignored -> {}, "1.0.2", testWebServer.getEndpoint(), configDir, "tool name"); - Assert.assertFalse(message.isPresent()); - } - @Test public void testPerformUpdateCheck_badLastUpdateTime() throws IOException { Instant before = Instant.now(); @@ -218,7 +186,7 @@ public void testPerformUpdateCheck_badLastUpdateTime() throws IOException { configDir.resolve("lastUpdateCheck"), "bad timestamp".getBytes(StandardCharsets.UTF_8)); Optional message = UpdateChecker.performUpdateCheck( - ignored -> {}, "1.0.2", testWebServer.getEndpoint(), configDir, "tool name"); + configDir, "1.0.2", versionUrl, "tool name", ignored -> {}); String modifiedTime = new String( Files.readAllBytes(configDir.resolve("lastUpdateCheck")), StandardCharsets.UTF_8); @@ -240,15 +208,15 @@ public void testPerformUpdateCheck_failSilently() 1)) { Optional message = UpdateChecker.performUpdateCheck( + configDir, + "1.0.2", + badServer.getEndpoint(), + "tool name", logEvent -> { Assert.assertEquals(Level.DEBUG, logEvent.getLevel()); MatcherAssert.assertThat( logEvent.getMessage(), CoreMatchers.containsString("Update check failed; ")); - }, - "1.0.2", - badServer.getEndpoint(), - configDir, - "tool name"); + }); Assert.assertFalse(message.isPresent()); } } @@ -271,10 +239,7 @@ public void testFinishUpdateCheck_notDone() { Assert.assertFalse(result.isPresent()); } - private void setupConfigAndLastUpdateCheck() throws IOException { - Files.write( - configDir.resolve("config.json"), - "{\"disableUpdateCheck\":false}".getBytes(StandardCharsets.UTF_8)); + private void setupLastUpdateCheck() throws IOException { Files.write( configDir.resolve("lastUpdateCheck"), Instant.now().minus(Duration.ofDays(2)).toString().getBytes(StandardCharsets.UTF_8)); diff --git a/jib-plugins-common/src/test/java/com/google/cloud/tools/jib/plugins/common/globalconfig/GlobalConfigTest.java b/jib-plugins-common/src/test/java/com/google/cloud/tools/jib/plugins/common/globalconfig/GlobalConfigTest.java new file mode 100644 index 0000000000..af3a776ca2 --- /dev/null +++ b/jib-plugins-common/src/test/java/com/google/cloud/tools/jib/plugins/common/globalconfig/GlobalConfigTest.java @@ -0,0 +1,79 @@ +/* + * Copyright 2021 Google LLC. + * + * 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.plugins.common.globalconfig; + +import static com.google.common.truth.Truth.assertThat; +import static org.junit.Assert.assertThrows; + +import com.google.cloud.tools.jib.plugins.common.PropertyNames; +import java.io.File; +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.contrib.java.lang.system.RestoreSystemProperties; +import org.junit.rules.TemporaryFolder; + +/** Tests for {@link GlobalConfig}. */ +public class GlobalConfigTest { + + @Rule public final RestoreSystemProperties systemPropertyRestorer = new RestoreSystemProperties(); + @Rule public final TemporaryFolder temporaryFolder = new TemporaryFolder(); + + private Path configDir; + + @Before + public void setUp() { + configDir = temporaryFolder.getRoot().toPath(); + } + + @Test + public void testIsDisableUpdateCheck_systemProperty() throws IOException { + Files.write( + configDir.resolve("config.json"), + "{\"disableUpdateCheck\":false}".getBytes(StandardCharsets.UTF_8)); + + GlobalConfig globalConfig = GlobalConfig.readConfig(configDir); + System.setProperty(PropertyNames.DISABLE_UPDATE_CHECKS, "true"); + assertThat(globalConfig.isDisableUpdateCheck()).isTrue(); + } + + @Test + public void testIsDisableUpdateCheck_disabled() throws IOException { + Files.write( + configDir.resolve("config.json"), + "{\"disableUpdateCheck\":true}".getBytes(StandardCharsets.UTF_8)); + + GlobalConfig globalConfig = GlobalConfig.readConfig(configDir); + assertThat(globalConfig.isDisableUpdateCheck()).isTrue(); + } + + @Test + public void testPerformUpdateCheck_badConfig() throws IOException { + Files.write( + configDir.resolve("config.json"), "corrupt config".getBytes(StandardCharsets.UTF_8)); + IOException exception = + assertThrows(IOException.class, () -> GlobalConfig.readConfig(configDir)); + assertThat(exception) + .hasMessageThat() + .startsWith("Failed to read global Jib config; you may need to fix or delete"); + assertThat(exception).hasMessageThat().endsWith(File.separator + "config.json"); + } +} From a2a202ab3b92df562888105e107a20d4d3aaf12a Mon Sep 17 00:00:00 2001 From: Chanseok Oh Date: Tue, 12 Jan 2021 16:49:06 -0500 Subject: [PATCH 3/5] Add tests --- .../common/globalconfig/GlobalConfigTest.java | 44 ++++++++++++++++--- 1 file changed, 38 insertions(+), 6 deletions(-) diff --git a/jib-plugins-common/src/test/java/com/google/cloud/tools/jib/plugins/common/globalconfig/GlobalConfigTest.java b/jib-plugins-common/src/test/java/com/google/cloud/tools/jib/plugins/common/globalconfig/GlobalConfigTest.java index af3a776ca2..c68d4a6413 100644 --- a/jib-plugins-common/src/test/java/com/google/cloud/tools/jib/plugins/common/globalconfig/GlobalConfigTest.java +++ b/jib-plugins-common/src/test/java/com/google/cloud/tools/jib/plugins/common/globalconfig/GlobalConfigTest.java @@ -45,28 +45,60 @@ public void setUp() { } @Test - public void testIsDisableUpdateCheck_systemProperty() throws IOException { + public void testReadConfig_default() throws IOException { + GlobalConfig globalConfig = GlobalConfig.readConfig(configDir); + assertThat(globalConfig.isDisableUpdateCheck()).isFalse(); + } + + @Test + public void testReadConfig_newConfigCreated() throws IOException { + GlobalConfig.readConfig(configDir); + String configJson = + new String(Files.readAllBytes(configDir.resolve("config.json")), StandardCharsets.UTF_8); + assertThat(configJson).isEqualTo("{\"disableUpdateCheck\":false}"); + } + + @Test + public void testReadConfig_emptyJson() throws IOException { + Files.write(configDir.resolve("config.json"), "{}".getBytes(StandardCharsets.UTF_8)); + GlobalConfig globalConfig = GlobalConfig.readConfig(configDir); + assertThat(globalConfig.isDisableUpdateCheck()).isFalse(); + } + + @Test + public void testReadConfig() throws IOException { Files.write( configDir.resolve("config.json"), - "{\"disableUpdateCheck\":false}".getBytes(StandardCharsets.UTF_8)); + "{\"disableUpdateCheck\":true}".getBytes(StandardCharsets.UTF_8)); GlobalConfig globalConfig = GlobalConfig.readConfig(configDir); - System.setProperty(PropertyNames.DISABLE_UPDATE_CHECKS, "true"); assertThat(globalConfig.isDisableUpdateCheck()).isTrue(); } @Test - public void testIsDisableUpdateCheck_disabled() throws IOException { + public void testReadConfig_systemProperties() throws IOException { Files.write( configDir.resolve("config.json"), - "{\"disableUpdateCheck\":true}".getBytes(StandardCharsets.UTF_8)); + "{\"disableUpdateCheck\":false}".getBytes(StandardCharsets.UTF_8)); GlobalConfig globalConfig = GlobalConfig.readConfig(configDir); + System.setProperty(PropertyNames.DISABLE_UPDATE_CHECKS, "true"); assertThat(globalConfig.isDisableUpdateCheck()).isTrue(); } @Test - public void testPerformUpdateCheck_badConfig() throws IOException { + public void testReadConfig_emptyFile() throws IOException { + temporaryFolder.newFile("config.json"); + IOException exception = + assertThrows(IOException.class, () -> GlobalConfig.readConfig(configDir)); + assertThat(exception) + .hasMessageThat() + .startsWith("Failed to read global Jib config; you may need to fix or delete"); + assertThat(exception).hasMessageThat().endsWith(File.separator + "config.json"); + } + + @Test + public void testReadConfig_corrupted() throws IOException { Files.write( configDir.resolve("config.json"), "corrupt config".getBytes(StandardCharsets.UTF_8)); IOException exception = From 196d077def6e98f74ee2de3ff6da26517a74a0a9 Mon Sep 17 00:00:00 2001 From: Chanseok Oh Date: Tue, 12 Jan 2021 17:13:48 -0500 Subject: [PATCH 4/5] wip --- .../jib/plugins/common/UpdateChecker.java | 4 +-- .../common/globalconfig/GlobalConfig.java | 2 +- .../globalconfig/GlobalConfigTemplate.java | 16 +++++++++++ .../jib/plugins/common/UpdateCheckerTest.java | 28 +++++++------------ 4 files changed, 29 insertions(+), 21 deletions(-) diff --git a/jib-plugins-common/src/main/java/com/google/cloud/tools/jib/plugins/common/UpdateChecker.java b/jib-plugins-common/src/main/java/com/google/cloud/tools/jib/plugins/common/UpdateChecker.java index 2ff32defb0..90a4840178 100644 --- a/jib-plugins-common/src/main/java/com/google/cloud/tools/jib/plugins/common/UpdateChecker.java +++ b/jib-plugins-common/src/main/java/com/google/cloud/tools/jib/plugins/common/UpdateChecker.java @@ -80,9 +80,9 @@ static Optional performUpdateCheck( String versionUrl, String toolName, Consumer log) { - try { - Path lastUpdateCheck = configDir.resolve(LAST_UPDATE_CHECK_FILENAME); + Path lastUpdateCheck = configDir.resolve(LAST_UPDATE_CHECK_FILENAME); + try { // Check time of last update check if (Files.exists(lastUpdateCheck)) { try { diff --git a/jib-plugins-common/src/main/java/com/google/cloud/tools/jib/plugins/common/globalconfig/GlobalConfig.java b/jib-plugins-common/src/main/java/com/google/cloud/tools/jib/plugins/common/globalconfig/GlobalConfig.java index 9fec7cc44b..aae6c387b3 100644 --- a/jib-plugins-common/src/main/java/com/google/cloud/tools/jib/plugins/common/globalconfig/GlobalConfig.java +++ b/jib-plugins-common/src/main/java/com/google/cloud/tools/jib/plugins/common/globalconfig/GlobalConfig.java @@ -85,7 +85,7 @@ public static Path getConfigDir() { return XdgDirectories.getConfigHome(); } - private GlobalConfigTemplate jsonConfig; + private final GlobalConfigTemplate jsonConfig; private GlobalConfig(GlobalConfigTemplate jsonConfig) { this.jsonConfig = jsonConfig; diff --git a/jib-plugins-common/src/main/java/com/google/cloud/tools/jib/plugins/common/globalconfig/GlobalConfigTemplate.java b/jib-plugins-common/src/main/java/com/google/cloud/tools/jib/plugins/common/globalconfig/GlobalConfigTemplate.java index 424660dc9e..bd4378f6d5 100644 --- a/jib-plugins-common/src/main/java/com/google/cloud/tools/jib/plugins/common/globalconfig/GlobalConfigTemplate.java +++ b/jib-plugins-common/src/main/java/com/google/cloud/tools/jib/plugins/common/globalconfig/GlobalConfigTemplate.java @@ -1,3 +1,19 @@ +/* + * Copyright 2021 Google LLC. + * + * 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.plugins.common.globalconfig; import com.fasterxml.jackson.annotation.JsonIgnoreProperties; diff --git a/jib-plugins-common/src/test/java/com/google/cloud/tools/jib/plugins/common/UpdateCheckerTest.java b/jib-plugins-common/src/test/java/com/google/cloud/tools/jib/plugins/common/UpdateCheckerTest.java index 17d14ef9a9..ac6b61004c 100644 --- a/jib-plugins-common/src/test/java/com/google/cloud/tools/jib/plugins/common/UpdateCheckerTest.java +++ b/jib-plugins-common/src/test/java/com/google/cloud/tools/jib/plugins/common/UpdateCheckerTest.java @@ -53,19 +53,13 @@ public class UpdateCheckerTest { private TestWebServer testWebServer; private Path configDir; - private String versionUrl; @Before public void setUp() throws InterruptedException, GeneralSecurityException, URISyntaxException, IOException { - testWebServer = - new TestWebServer( - false, - Collections.singletonList( - "HTTP/1.1 200 OK\nContent-Length:18\n\n{\"latest\":\"2.0.0\"}"), - 1); + String response = "HTTP/1.1 200 OK\nContent-Length:18\n\n{\"latest\":\"2.0.0\"}"; + testWebServer = new TestWebServer(false, Collections.singletonList(response), 1); configDir = temporaryFolder.getRoot().toPath(); - versionUrl = testWebServer.getEndpoint(); } @After @@ -79,7 +73,7 @@ public void testPerformUpdateCheck_newVersionFound() throws IOException { setupLastUpdateCheck(); Optional message = UpdateChecker.performUpdateCheck( - configDir, "1.0.2", versionUrl, "tool name", ignored -> {}); + configDir, "1.0.2", testWebServer.getEndpoint(), "tool name", ignored -> {}); Assert.assertTrue(testWebServer.getInputRead().contains("User-Agent: jib 1.0.2 tool name")); Assert.assertTrue(message.isPresent()); Assert.assertEquals( @@ -116,7 +110,7 @@ public void testPerformUpdateCheck_onLatest() throws IOException { setupLastUpdateCheck(); Optional message = UpdateChecker.performUpdateCheck( - configDir, "2.0.0", versionUrl, "tool name", ignored -> {}); + configDir, "2.0.0", testWebServer.getEndpoint(), "tool name", ignored -> {}); Assert.assertFalse(message.isPresent()); String modifiedTime = new String( @@ -130,7 +124,7 @@ public void testPerformUpdateCheck_noLastUpdateCheck() throws IOException { Instant before = Instant.now(); Optional message = UpdateChecker.performUpdateCheck( - configDir, "1.0.2", versionUrl, "tool name", ignored -> {}); + configDir, "1.0.2", testWebServer.getEndpoint(), "tool name", ignored -> {}); Assert.assertTrue(message.isPresent()); Assert.assertEquals( "A new version of Jib (2.0.0) is available (currently using 1.0.2). Update your build " @@ -148,7 +142,7 @@ public void testPerformUpdateCheck_emptyLastUpdateCheck() throws IOException { Instant before = Instant.now(); Optional message = UpdateChecker.performUpdateCheck( - configDir, "1.0.2", versionUrl, "tool name", ignored -> {}); + configDir, "1.0.2", testWebServer.getEndpoint(), "tool name", ignored -> {}); Assert.assertTrue(message.isPresent()); Assert.assertEquals( "A new version of Jib (2.0.0) is available (currently using 1.0.2). Update your build " @@ -169,7 +163,7 @@ public void testPerformUpdateCheck_lastUpdateCheckTooSoon() throws IOException { modifiedTime.toString().getBytes(StandardCharsets.UTF_8)); Optional message = UpdateChecker.performUpdateCheck( - configDir, "1.0.2", versionUrl, "tool name", ignored -> {}); + configDir, "1.0.2", testWebServer.getEndpoint(), "tool name", ignored -> {}); Assert.assertFalse(message.isPresent()); // lastUpdateCheck should not have changed @@ -186,7 +180,7 @@ public void testPerformUpdateCheck_badLastUpdateTime() throws IOException { configDir.resolve("lastUpdateCheck"), "bad timestamp".getBytes(StandardCharsets.UTF_8)); Optional message = UpdateChecker.performUpdateCheck( - configDir, "1.0.2", versionUrl, "tool name", ignored -> {}); + configDir, "1.0.2", testWebServer.getEndpoint(), "tool name", ignored -> {}); String modifiedTime = new String( Files.readAllBytes(configDir.resolve("lastUpdateCheck")), StandardCharsets.UTF_8); @@ -201,11 +195,9 @@ public void testPerformUpdateCheck_badLastUpdateTime() throws IOException { @Test public void testPerformUpdateCheck_failSilently() throws InterruptedException, GeneralSecurityException, URISyntaxException, IOException { + String response = "HTTP/1.1 400 Bad Request\nContent-Length: 0\n\n"; try (TestWebServer badServer = - new TestWebServer( - false, - Collections.singletonList("HTTP/1.1 400 Bad Request\nContent-Length: 0\n\n"), - 1)) { + new TestWebServer(false, Collections.singletonList(response), 1)) { Optional message = UpdateChecker.performUpdateCheck( configDir, From 382d0f89c962ba5af225902361ff42cd26750a6f Mon Sep 17 00:00:00 2001 From: Chanseok Oh Date: Wed, 13 Jan 2021 14:56:35 -0500 Subject: [PATCH 5/5] Clean up --- .../cloud/tools/jib/maven/BuildDockerMojo.java | 12 ++++-------- .../google/cloud/tools/jib/maven/BuildImageMojo.java | 12 ++++-------- .../google/cloud/tools/jib/maven/BuildTarMojo.java | 12 ++++-------- 3 files changed, 12 insertions(+), 24 deletions(-) diff --git a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildDockerMojo.java b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildDockerMojo.java index dec33eeb97..c7336dcf57 100644 --- a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildDockerMojo.java +++ b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildDockerMojo.java @@ -36,6 +36,7 @@ import com.google.cloud.tools.jib.plugins.extension.JibPluginExtensionException; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; +import com.google.common.util.concurrent.Futures; import java.io.IOException; import java.nio.file.Path; import java.util.Optional; @@ -84,16 +85,11 @@ public void execute() throws MojoExecutionException, MojoFailureException { getLog(), tempDirectoryProvider); - GlobalConfig globalConfig = null; + Future> updateCheckFuture = Futures.immediateFuture(Optional.empty()); try { - globalConfig = GlobalConfig.readConfig(); - } catch (IOException ex) { - throw new MojoExecutionException(ex.getMessage(), ex); - } - Future> updateCheckFuture = - MojoCommon.newUpdateChecker(projectProperties, globalConfig, getLog()); + updateCheckFuture = + MojoCommon.newUpdateChecker(projectProperties, GlobalConfig.readConfig(), getLog()); - try { PluginConfigurationProcessor.createJibBuildRunnerForDockerDaemonImage( new MavenRawConfiguration(this), new MavenSettingsServerCredentials( diff --git a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildImageMojo.java b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildImageMojo.java index f105713459..6d4d8ba635 100644 --- a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildImageMojo.java +++ b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildImageMojo.java @@ -37,6 +37,7 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.base.Strings; +import com.google.common.util.concurrent.Futures; import java.io.IOException; import java.util.Arrays; import java.util.Optional; @@ -98,16 +99,11 @@ public void execute() throws MojoExecutionException, MojoFailureException { getLog(), tempDirectoryProvider); - GlobalConfig globalConfig = null; + Future> updateCheckFuture = Futures.immediateFuture(Optional.empty()); try { - globalConfig = GlobalConfig.readConfig(); - } catch (IOException ex) { - throw new MojoExecutionException(ex.getMessage(), ex); - } - Future> updateCheckFuture = - MojoCommon.newUpdateChecker(projectProperties, globalConfig, getLog()); + updateCheckFuture = + MojoCommon.newUpdateChecker(projectProperties, GlobalConfig.readConfig(), getLog()); - try { PluginConfigurationProcessor.createJibBuildRunnerForRegistryImage( new MavenRawConfiguration(this), new MavenSettingsServerCredentials( diff --git a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildTarMojo.java b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildTarMojo.java index 959dc2fd07..396a74e8aa 100644 --- a/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildTarMojo.java +++ b/jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildTarMojo.java @@ -35,6 +35,7 @@ import com.google.cloud.tools.jib.plugins.extension.JibPluginExtensionException; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; +import com.google.common.util.concurrent.Futures; import java.io.IOException; import java.util.Optional; import java.util.concurrent.Future; @@ -76,16 +77,11 @@ public void execute() throws MojoExecutionException, MojoFailureException { getLog(), tempDirectoryProvider); - GlobalConfig globalConfig = null; + Future> updateCheckFuture = Futures.immediateFuture(Optional.empty()); try { - globalConfig = GlobalConfig.readConfig(); - } catch (IOException ex) { - throw new MojoExecutionException(ex.getMessage(), ex); - } - Future> updateCheckFuture = - MojoCommon.newUpdateChecker(projectProperties, globalConfig, getLog()); + updateCheckFuture = + MojoCommon.newUpdateChecker(projectProperties, GlobalConfig.readConfig(), getLog()); - try { PluginConfigurationProcessor.createJibBuildRunnerForTarImage( new MavenRawConfiguration(this), new MavenSettingsServerCredentials(