Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate jib.extraDirectory in Gradle #1671

Merged
merged 12 commits into from
May 3, 2019
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ static void buildAndRunAdditionalTag(
assertCreationTimeEpoch(additionalImageReference);
}

static void buildToDockerDaemon(
static BuildResult buildToDockerDaemon(
TestProject testProject, String imageReference, String gradleBuildFile)
throws IOException, InterruptedException, DigestException {
BuildResult buildResult =
Expand All @@ -106,6 +106,8 @@ static void buildToDockerDaemon(

String history = new Command("docker", "history", imageReference).run();
Assert.assertThat(history, CoreMatchers.containsString("jib-gradle-plugin"));

return buildResult;
}

static String buildToDockerDaemonAndRun(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,22 @@ private static void assertDockerInspect(String imageReference)
+ " }"));
}

private static void assertExtraDirectoryDeprecationWarning(String pomXml)
throws DigestException, IOException, InterruptedException {
String targetImage = "localhost:6000/simpleimage:gradle" + System.nanoTime();
BuildResult buildResult =
JibRunHelper.buildToDockerDaemon(simpleTestProject, targetImage, pomXml);
Assert.assertEquals(
"Hello, world. \nrw-r--r--\nrw-r--r--\nfoo\ncat\n",
new Command("docker", "run", "--rm", targetImage).run());
Assert.assertThat(
buildResult.getOutput(),
CoreMatchers.containsString(
"'jib.extraDirectory', 'jib.extraDirectory.path', and 'jib.extraDirectory.permissions' "
+ "are deprecated; use 'jib.extraDirectories', 'jib.extraDirectories.paths' and "
chanseokoh marked this conversation as resolved.
Show resolved Hide resolved
+ "'jib.extraDirectories.permissions'"));
}

private static String buildAndRunComplex(
String imageReference, String username, String password, LocalRegistry targetRegistry)
throws IOException, InterruptedException {
Expand Down Expand Up @@ -232,6 +248,24 @@ public void testDockerDaemon_simpleWithIncompatibleJava11()
}
}

@Test
public void testDockerDaemon_simple_deprecatedExtraDirectory()
throws DigestException, IOException, InterruptedException {
assertExtraDirectoryDeprecationWarning("build-extra-dir-deprecated.gradle");
}

@Test
public void testDockerDaemon_simple_deprecatedExtraDirectory2()
throws DigestException, IOException, InterruptedException {
assertExtraDirectoryDeprecationWarning("build-extra-dir-deprecated.gradle2");
}

@Test
public void testDockerDaemon_simple_deprecatedExtraDirectory3()
throws DigestException, IOException, InterruptedException {
assertExtraDirectoryDeprecationWarning("build-extra-dir-deprecated.gradle3");
}

@Test
public void testDockerDaemon_simple_multipleExtraDirectories()
throws DigestException, IOException, InterruptedException {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
plugins {
id 'java'
id 'com.google.cloud.tools.jib'
}

sourceCompatibility = 1.8
targetCompatibility = 1.8

repositories {
mavenCentral()
}

dependencies {
compile files('libs/dependency-1.0.0.jar')
}

jib.to.image = System.getProperty("_TARGET_IMAGE")
jib.extraDirectory {
path = file('src/main/custom-extra-dir')
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
plugins {
id 'java'
id 'com.google.cloud.tools.jib'
}

sourceCompatibility = 1.8
targetCompatibility = 1.8

repositories {
mavenCentral()
}

dependencies {
compile files('libs/dependency-1.0.0.jar')
}

jib.to.image = System.getProperty("_TARGET_IMAGE")
jib.extraDirectory = file('src/main/custom-extra-dir')
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
plugins {
id 'java'
id 'com.google.cloud.tools.jib'
}

sourceCompatibility = 1.8
targetCompatibility = 1.8

repositories {
mavenCentral()
}

dependencies {
compile files('libs/dependency-1.0.0.jar')
}

jib.to.image = System.getProperty("_TARGET_IMAGE")
jib.extraDirectory.path = file('src/main/custom-extra-dir')
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,4 @@ dependencies {
}

jib.to.image = System.getProperty("_TARGET_IMAGE")
jib.extraDirectory = ['src/main/custom-extra-dir', 'src/main/custom-extra-dir2']
jib.extraDirectories.paths = ['src/main/custom-extra-dir', 'src/main/custom-extra-dir2']
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ dependencies {
}

jib.to.image = System.getProperty("_TARGET_IMAGE")
jib.extraDirectory {
path = files('src/main/custom-extra-dir', 'src/main/custom-extra-dir2')
jib.extraDirectories {
paths = files('src/main/custom-extra-dir', 'src/main/custom-extra-dir2')
permissions = ['/baz':'705']
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,5 +28,5 @@ jib {
workingDirectory = '/home'
extraClasspath = ['/d1','/d2']
}
extraDirectory = file('src/main/custom-extra-dir')
extraDirectories.paths = file('src/main/custom-extra-dir')
}
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,8 @@ jib {
labels = [key1:'value1', key2:'value2']
volumes = ['/var/log', '/var/log2']
}
extraDirectory {
path = file('src/main/custom-extra-dir')
extraDirectories {
paths = file('src/main/custom-extra-dir')
permissions = ['/foo':'755', '/bar/cat':'777']
}
allowInsecureRegistries = true
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ public void buildDocker()

// Asserts required @Input parameters are not null.
Preconditions.checkNotNull(jibExtension);
TaskCommon.checkDeprecatedUsage(jibExtension, getLogger());
TaskCommon.disableHttpLogging();

try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ public void buildImage()
MainClassInferenceException {
// Asserts required @Input parameters are not null.
Preconditions.checkNotNull(jibExtension);
TaskCommon.checkDeprecatedUsage(jibExtension, getLogger());
TaskCommon.disableHttpLogging();

try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ public void setTargetImage(String targetImage) {
@InputFiles
public FileCollection getInputFiles() {
List<Path> extraDirectories =
Preconditions.checkNotNull(jibExtension).getExtraDirectory().getPaths();
Preconditions.checkNotNull(jibExtension).getExtraDirectories().getPaths();
return extraDirectories
.stream()
.map(Path::toFile)
Expand All @@ -105,6 +105,7 @@ public void buildTar()
MainClassInferenceException {
// Asserts required @Input parameters are not null.
Preconditions.checkNotNull(jibExtension);
TaskCommon.checkDeprecatedUsage(jibExtension, getLogger());
TaskCommon.disableHttpLogging();

try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,17 +30,19 @@
import org.gradle.api.tasks.Input;
import org.gradle.api.tasks.Internal;

/** Object in {@link JibExtension} that configures the extra directory. */
public class ExtraDirectoryParameters {
/** Object in {@link JibExtension} that configures the extra directories. */
public class ExtraDirectoriesParameters {

private final Project project;
@Deprecated private final JibExtension jibExtension;

private List<Path> paths;
private Map<String, String> permissions = Collections.emptyMap();

@Inject
public ExtraDirectoryParameters(Project project) {
public ExtraDirectoriesParameters(Project project, JibExtension jibExtension) {
this.project = project;
this.jibExtension = jibExtension;
paths =
Collections.singletonList(
project.getProjectDir().toPath().resolve("src").resolve("main").resolve("jib"));
Expand All @@ -57,7 +59,9 @@ public List<String> getPathStrings() {
public List<Path> getPaths() {
// Gradle warns about @Input annotations on File objects, so we have to expose a getter for a
// String to make them go away.
String property = System.getProperty(PropertyNames.EXTRA_DIRECTORY_PATH);
String deprecatedProperty = System.getProperty(PropertyNames.EXTRA_DIRECTORY_PATH);
String newProperty = System.getProperty(PropertyNames.EXTRA_DIRECTORIES_PATHS);
String property = newProperty != null ? newProperty : deprecatedProperty;
if (property != null) {
List<String> pathStrings = ConfigurationPropertyValidator.parseListProperty(property);
return pathStrings.stream().map(Paths::get).collect(Collectors.toList());
Expand All @@ -67,16 +71,22 @@ public List<Path> getPaths() {

/**
* Sets paths. {@code paths} can be any suitable object describing file paths convertible by
* {@link Project#files} (such as {@code List<File>}).
* {@link Project#files} (such as {@link File}, {@code List<File>}, or {@code List<String>}).
*
* @param paths paths to set.
*/
// non-plural to retain backward-compatibility for the "jib.extraDirectory.path" config parameter
public void setPath(Object paths) {
public void setPaths(Object paths) {
jibExtension.extraDirectoriesConfigured = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious how necessary this is, would prefer we didn't need to keep track of things with flags?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is quite unfortunate. The Gradle case wasn't easy for a few reasons.

  1. I couldn't find a way (if at all) to log inside JibExtension or ExtraDirectoriesParameters.
  2. The way we can extract extraDirectories.paths or extraDirectory.path in our codebase for consumption is to first go through getExtraDirectories(), which is also used by Gradle to set in config values, so unlike Maven, it's not possible to check if the user is using the old or new syntax (or both) inside getExtraDirectories(). On top of that, Gradle always calls both getExtraDirectory() and getExtraDirectories() regardless of whether the user has ever configured anything.

So, although this is very unfortunate, I couldn't find a good alternative.

this.paths =
project.files(paths).getFiles().stream().map(File::toPath).collect(Collectors.toList());
}

@Deprecated
public void setPath(File path) {
jibExtension.extraDirectoryConfigured = true;
this.paths = Collections.singletonList(path.toPath());
}

/**
* Gets the permissions for files in the extra layer on the container. Maps from absolute path on
* the container to a 3-digit octal string representation of the file permission bits (e.g. {@code
Expand All @@ -86,9 +96,11 @@ public void setPath(Object paths) {
*/
@Input
public Map<String, String> getPermissions() {
if (System.getProperty(PropertyNames.EXTRA_DIRECTORY_PERMISSIONS) != null) {
return ConfigurationPropertyValidator.parseMapProperty(
System.getProperty(PropertyNames.EXTRA_DIRECTORY_PERMISSIONS));
String deprecatedProperty = System.getProperty(PropertyNames.EXTRA_DIRECTORY_PERMISSIONS);
String newProperty = System.getProperty(PropertyNames.EXTRA_DIRECTORIES_PERMISSIONS);
String property = newProperty != null ? newProperty : deprecatedProperty;
if (property != null) {
return ConfigurationPropertyValidator.parseMapProperty(property);
}
return permissions;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ public void listFiles() {
printProjectFiles(project);

// Print extra layer
List<Path> extraDirectories = jibExtension.getExtraDirectory().getPaths();
List<Path> extraDirectories = jibExtension.getExtraDirectories().getPaths();
extraDirectories.stream().filter(Files::exists).forEach(System.out::println);

// Find project dependencies
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ public void listFiles() throws IOException {
addProjectFiles(project);

// Add extra layer
List<Path> extraDirectories = jibExtension.getExtraDirectory().getPaths();
List<Path> extraDirectories = jibExtension.getExtraDirectories().getPaths();
extraDirectories.stream().filter(Files::exists).forEach(skaffoldFilesOutput::addInput);

// Find project dependencies
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,11 +152,11 @@ public Optional<String> getProperty(String propertyName) {

@Override
public List<Path> getExtraDirectories() {
return jibExtension.getExtraDirectory().getPaths();
return jibExtension.getExtraDirectories().getPaths();
}

@Override
public Map<AbsoluteUnixPath, FilePermissions> getExtraDirectoryPermissions() {
return TaskCommon.convertPermissionsMap(jibExtension.getExtraDirectory().getPermissions());
return TaskCommon.convertPermissionsMap(jibExtension.getExtraDirectories().getPermissions());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package com.google.cloud.tools.jib.gradle;

import com.google.cloud.tools.jib.plugins.common.PropertyNames;
import java.io.File;
import org.gradle.api.Action;
import org.gradle.api.Project;
import org.gradle.api.model.ObjectFactory;
Expand Down Expand Up @@ -48,8 +49,8 @@
* format = OCI
* appRoot = '/app'
* }
* extraDirectory {
* path = file('path/to/extra/dir')
* extraDirectories {
* paths = ['/path/to/extra/dir', 'can/be/relative/to/project/root']
* permissions = [
* '/path/on/container/file1': 744,
* '/path/on/container/file2': 123
Expand All @@ -67,17 +68,19 @@ public class JibExtension {
private final BaseImageParameters from;
private final TargetImageParameters to;
private final ContainerParameters container;
private final ExtraDirectoryParameters extraDirectory;

private final ExtraDirectoriesParameters extraDirectories;
private final Property<Boolean> allowInsecureRegistries;

@Deprecated boolean extraDirectoryConfigured;
@Deprecated boolean extraDirectoriesConfigured;

public JibExtension(Project project) {
ObjectFactory objectFactory = project.getObjects();

from = objectFactory.newInstance(BaseImageParameters.class);
to = objectFactory.newInstance(TargetImageParameters.class);
container = objectFactory.newInstance(ContainerParameters.class);
extraDirectory = objectFactory.newInstance(ExtraDirectoryParameters.class, project);
extraDirectories = objectFactory.newInstance(ExtraDirectoriesParameters.class, project, this);

allowInsecureRegistries = objectFactory.property(Boolean.class);

Expand All @@ -97,19 +100,22 @@ public void container(Action<? super ContainerParameters> action) {
action.execute(container);
}

public void extraDirectory(Action<? super ExtraDirectoryParameters> action) {
action.execute(extraDirectory);
@Deprecated
public void extraDirectory(Action<? super ExtraDirectoriesParameters> action) {
extraDirectoryConfigured = true;
action.execute(extraDirectories);
}

public void extraDirectories(Action<? super ExtraDirectoriesParameters> action) {
extraDirectoriesConfigured = true;
action.execute(extraDirectories);
}

/**
* Sets extra directory paths. {@code extraDirectories} can be any suitable object describing file
* paths convertible by {@link Project#files} (such as {@code List<File>}).
*
* @param extraDirectories paths to set.
*/
// non-plural to retain backward-compatibility for the "jib.extraDirectory" config parameter
public void setExtraDirectory(Object extraDirectories) {
this.extraDirectory.setPath(extraDirectories);
@Deprecated
// for the deprecated "jib.extraDirectory" config parameter
public void setExtraDirectory(File extraDirectory) {
extraDirectoryConfigured = true;
this.extraDirectories.setPath(extraDirectory);
}

public void setAllowInsecureRegistries(boolean allowInsecureRegistries) {
Expand All @@ -134,10 +140,17 @@ public ContainerParameters getContainer() {
return container;
}

@Deprecated
@Nested
@Optional
public ExtraDirectoriesParameters getExtraDirectory() {
return extraDirectories;
}

@Nested
@Optional
public ExtraDirectoryParameters getExtraDirectory() {
return extraDirectory;
public ExtraDirectoriesParameters getExtraDirectories() {
return extraDirectories;
}

@Input
Expand Down
Loading