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

Use Gradle Property and Provider to enable lazy evaluation for jib.extraDirectories parameters #3737

Merged
merged 24 commits into from
Aug 25, 2022
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
a55e53a
Update permissions type from Map to MapProperty
emmileaf Aug 16, 2022
019fd65
Convert path.from and path.into to property types
emmileaf Aug 16, 2022
3402b29
WIP: add/fix unit tests
emmileaf Aug 16, 2022
63319af
WIP: changes to setFrom to accept Provider type
emmileaf Aug 17, 2022
5e6f237
WIP: comment out added tests for more debugging
emmileaf Aug 17, 2022
a5f1aee
WIP: clean up updated setFrom to allow for lazy eval
emmileaf Aug 17, 2022
56cb69e
WIP: fix added unit test, can lazy eval using setFrom but not setPaths
emmileaf Aug 17, 2022
b43c295
Add extra directories to lazy evaluation testing project
emmileaf Aug 17, 2022
7370713
Switch setFrom to method overloading
emmileaf Aug 18, 2022
1360011
Update setPaths to allow for lazy eval
emmileaf Aug 18, 2022
1819eb3
Update unit tests to check for both specification options
emmileaf Aug 18, 2022
b2815b7
Remove setPermissions test
emmileaf Aug 19, 2022
798882d
Fix file formatting
emmileaf Aug 19, 2022
b79d5ec
Switch from anonymous inner classes to lambdas
emmileaf Aug 22, 2022
925d5c3
Fix test compatibility with windows
emmileaf Aug 22, 2022
af4bb2c
Coverage: add tests for extraDirectories in JibExtensionTest
emmileaf Aug 23, 2022
4d986fa
Rename test file for committing test extra directory
emmileaf Aug 24, 2022
bd9183d
Update changelog and readme
emmileaf Aug 24, 2022
11d9dd1
Move object to parameters logic into helper method
emmileaf Aug 24, 2022
c7e131e
Fix javadoc checkstyle violations
emmileaf Aug 24, 2022
f380733
Rename added tests
emmileaf Aug 24, 2022
96197cf
Rename helper method
emmileaf Aug 24, 2022
25e47a5
Updated test to use stricter assert statement for path.from
emmileaf Aug 25, 2022
6f0a3b7
Update test file with more informative content
emmileaf Aug 25, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@
import org.gradle.api.Project;
import org.gradle.api.model.ObjectFactory;
import org.gradle.api.provider.ListProperty;
import org.gradle.api.provider.MapProperty;
import org.gradle.api.provider.Provider;
import org.gradle.api.tasks.Input;
import org.gradle.api.tasks.Internal;

Expand All @@ -40,14 +42,15 @@ public class ExtraDirectoriesParameters {

private ListProperty<ExtraDirectoryParameters> paths;
private ExtraDirectoryParametersSpec spec;
private Map<String, String> permissions = Collections.emptyMap();
private MapProperty<String, String> permissions;

@Inject
public ExtraDirectoriesParameters(ObjectFactory objects, Project project) {
this.objects = objects;
this.project = project;
paths = objects.listProperty(ExtraDirectoryParameters.class).empty();
spec = objects.newInstance(ExtraDirectoryParametersSpec.class, project, paths);
permissions = objects.mapProperty(String.class, String.class).empty();
}

public void paths(Action<? super ExtraDirectoryParametersSpec> action) {
Expand Down Expand Up @@ -98,6 +101,22 @@ public void setPaths(Object paths) {
.collect(Collectors.toList()));
}

/**
* Sets paths, for lazy evaluation where {@code paths} is a {@link Provider} of a suitable object.
*
* @param paths provider of paths to set
* @see #setPaths(Object)
*/
public void setPaths(Provider<Object> paths) {
mpeddada1 marked this conversation as resolved.
Show resolved Hide resolved
// Convert Provider<Object> to Provider<List<ExtraDirectoryParameters>>
mpeddada1 marked this conversation as resolved.
Show resolved Hide resolved
this.paths.set(
paths.map(
obj ->
project.files(paths).getFiles().stream()
.map(file -> new ExtraDirectoryParameters(objects, project, file.toPath(), "/"))
mpeddada1 marked this conversation as resolved.
Show resolved Hide resolved
.collect(Collectors.toList())));
}

/**
* 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 @@ -106,15 +125,15 @@ public void setPaths(Object paths) {
* @return the permissions map from path on container to file permissions
*/
@Input
public Map<String, String> getPermissions() {
public MapProperty<String, String> getPermissions() {
String property = System.getProperty(PropertyNames.EXTRA_DIRECTORIES_PERMISSIONS);
if (property != null) {
return ConfigurationPropertyValidator.parseMapProperty(property);
Map<String, String> parsedPermissions =
ConfigurationPropertyValidator.parseMapProperty(property);
if (!parsedPermissions.equals(permissions.get())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this check even matter? There is no harm in setting permissions to an equivalent object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True! I saw that both PR #3242 and #3034 both included this check in the getter, and my understanding was that we wanted to avoid setting a property multiple times here?

permissions.set(parsedPermissions);
}
}
return permissions;
}

public void setPermissions(Map<String, String> permissions) {
this.permissions = permissions;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,56 +24,69 @@
import org.gradle.api.Project;
import org.gradle.api.model.ObjectFactory;
import org.gradle.api.provider.ListProperty;
import org.gradle.api.provider.Property;
import org.gradle.api.provider.Provider;
import org.gradle.api.tasks.Input;
import org.gradle.api.tasks.Internal;

/** Configuration of an extra directory. */
public class ExtraDirectoryParameters implements ExtraDirectoriesConfiguration {

private Project project;
private Path from = Paths.get("");
private String into = "/";
private Property<Path> from;
private Property<String> into;
private ListProperty<String> includes;
private ListProperty<String> excludes;

@Inject
public ExtraDirectoryParameters(ObjectFactory objects, Project project) {
this.project = project;
includes = objects.listProperty(String.class).empty();
excludes = objects.listProperty(String.class).empty();
this.from = objects.property(Path.class).value(Paths.get(""));
this.into = objects.property(String.class).value("/");
this.includes = objects.listProperty(String.class).empty();
this.excludes = objects.listProperty(String.class).empty();
}

ExtraDirectoryParameters(ObjectFactory objects, Project project, Path from, String into) {
this(objects, project);
this.from = from;
this.into = into;
this.from = objects.property(Path.class).value(from);
this.into = objects.property(String.class).value(into);
}

@Input
public String getFromString() {
// Gradle warns about @Input annotations on File objects, so we have to expose a getter for a
// String to make them go away.
return from.toString();
return from.get().toString();
}

@Override
@Internal
public Path getFrom() {
return from;
return from.get();
}

public void setFrom(Object from) {
this.from = project.file(from).toPath();
this.from.set(project.file(from).toPath());
}

public void setFrom(Provider<Object> from) {
// Convert Provider<Object> to Provider<Path>
this.from.set(from.map(obj -> project.file(obj).toPath()));
}

@Override
@Input
public String getInto() {
return into;
return into.get();
}

public void setInto(String into) {
this.into = into;
this.into.set(into);
}

public void setInto(Provider<String> into) {
this.into.set(into);
}

@Input
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,8 @@ public List<? extends ExtraDirectoriesConfiguration> getExtraDirectories() {

@Override
public Map<String, FilePermissions> getExtraDirectoryPermissions() {
return TaskCommon.convertPermissionsMap(jibExtension.getExtraDirectories().getPermissions());
return TaskCommon.convertPermissionsMap(
jibExtension.getExtraDirectories().getPermissions().get());
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@
import java.util.List;
import java.util.Properties;
import org.gradle.api.Project;
import org.gradle.api.provider.Provider;
import org.gradle.api.provider.ProviderFactory;
import org.gradle.testfixtures.ProjectBuilder;
import org.junit.Before;
import org.junit.Rule;
Expand Down Expand Up @@ -241,23 +243,42 @@ public void testExtraDirectories_default() {
assertThat(testJibExtension.getExtraDirectories().getPaths()).hasSize(1);
assertThat(testJibExtension.getExtraDirectories().getPaths().get(0).getFrom())
.isEqualTo(fakeProject.getProjectDir().toPath().resolve("src/main/jib"));
assertThat(testJibExtension.getExtraDirectories().getPermissions()).isEmpty();
assertThat(testJibExtension.getExtraDirectories().getPermissions().get()).isEmpty();
}

@Test
public void testExtraDirectories() {
testJibExtension.extraDirectories(
extraDirectories -> {
extraDirectories.setPaths("test/path");
extraDirectories.setPermissions(ImmutableMap.of("file1", "123", "file2", "456"));
});

assertThat(testJibExtension.getExtraDirectories().getPaths()).hasSize(1);
assertThat(testJibExtension.getExtraDirectories().getPaths().get(0).getFrom())
.isEqualTo(fakeProject.getProjectDir().toPath().resolve("test/path"));
assertThat(testJibExtension.getExtraDirectories().getPermissions())
.containsExactly("file1", "123", "file2", "456")
.inOrder();
}

@Test
public void testExtraDirectories_lazySetFromInto() {
emmileaf marked this conversation as resolved.
Show resolved Hide resolved
testJibExtension.extraDirectories(
extraDirectories ->
extraDirectories.paths(
paths -> {
ProviderFactory providerFactory = fakeProject.getProviders();
Provider<Object> from = providerFactory.provider(() -> "test/path");
Provider<String> into = providerFactory.provider(() -> "/target");
paths.path(
path -> {
path.setFrom(from);
path.setInto(into);
});
}));

assertThat(testJibExtension.getExtraDirectories().getPaths()).hasSize(1);
assertThat(testJibExtension.getExtraDirectories().getPaths().get(0).getFrom())
.isEqualTo(fakeProject.getProjectDir().toPath().resolve("test/path"));
assertThat(testJibExtension.getExtraDirectories().getPaths().get(0).getInto())
.isEqualTo("/target");
}

@Test
Expand Down Expand Up @@ -309,6 +330,23 @@ public void testExtraDirectories_stringListForPaths() {
.isEqualTo(fakeProject.getProjectDir().toPath().resolve("another/path"));
}

@Test
public void testExtraDirectories_lazyStringListForPaths() {
emmileaf marked this conversation as resolved.
Show resolved Hide resolved
testJibExtension.extraDirectories(
extraDirectories -> {
ProviderFactory providerFactory = fakeProject.getProviders();
Provider<Object> paths =
providerFactory.provider(() -> Arrays.asList("test/path", "another/path"));
extraDirectories.setPaths(paths);
});

assertThat(testJibExtension.getExtraDirectories().getPaths()).hasSize(2);
assertThat(testJibExtension.getExtraDirectories().getPaths().get(0).getFrom())
.isEqualTo(fakeProject.getProjectDir().toPath().resolve("test/path"));
assertThat(testJibExtension.getExtraDirectories().getPaths().get(1).getFrom())
.isEqualTo(fakeProject.getProjectDir().toPath().resolve("another/path"));
}

@Test
public void testExtraDirectories_fileListForPaths() {
testJibExtension.extraDirectories(
Expand Down Expand Up @@ -456,7 +494,7 @@ public void testProperties() {
assertThat(testJibExtension.getExtraDirectories().getPaths().get(1).getFrom())
.isEqualTo(Paths.get("/bar/baz"));
System.setProperty("jib.extraDirectories.permissions", "/foo/bar=707,/baz=456");
assertThat(testJibExtension.getExtraDirectories().getPermissions())
assertThat(testJibExtension.getExtraDirectories().getPermissions().get())
.containsExactly("/foo/bar", "707", "/baz", "456")
.inOrder();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -368,6 +368,28 @@ public void testLazyEvalForLabels() {
"labels contain values [firstkey:updated-first-label, secondKey:updated-second-label]");
}

@Test
public void testLazyEvalForExtraDirectories() {
BuildResult checkExtraDirectories =
testProject.build("check-extra-directories", "-Djib.console=plain");
assertThat(checkExtraDirectories.getOutput()).contains("[/updated:755]");
assertThat(checkExtraDirectories.getOutput()).contains("updated-custom-extra-dir");
}

@Test
public void testLazyEvalForExtraDirectories_IndividualPaths() {
emmileaf marked this conversation as resolved.
Show resolved Hide resolved
BuildResult checkExtraDirectories =
testProject.build(
"check-extra-directories", "-b=build-extra-dirs.gradle", "-Djib.console=plain");
assertThat(checkExtraDirectories.getOutput()).contains("updated-custom-extra-dir");
mpeddada1 marked this conversation as resolved.
Show resolved Hide resolved
assertThat(checkExtraDirectories.getOutput())
.contains("extraDirectories (into): [/updated-custom-into-dir]");
assertThat(checkExtraDirectories.getOutput())
.contains("extraDirectories (includes): [[include.txt]]");
assertThat(checkExtraDirectories.getOutput())
.contains("extraDirectories (excludes): [[exclude.txt]]");
}

private Project createProject(String... plugins) {
Project project =
ProjectBuilder.builder().withProjectDir(testProjectRoot.getRoot()).withName("root").build();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
plugins {
id 'java'
id 'com.google.cloud.tools.jib'
}

sourceCompatibility = 1.8
targetCompatibility = 1.8

repositories {
mavenCentral()
}

dependencies {
implementation 'com.google.guava:guava:23.6-jre'
}

project.ext.value = 'original'

project.afterEvaluate {
project.ext.value = 'updated'
project.ext.getCustomIncludes = { -> return ['include.txt'] }
project.ext.getCustomExcludes = { -> return ['exclude.txt'] }
}

jib {
extraDirectories {
paths {
path {
from = project.provider { 'src/main/' + project.ext.value + '-custom-extra-dir' }
into = project.provider { '/' + project.ext.value + '-custom-into-dir' }
includes = project.provider { -> project.ext.getCustomIncludes() }
excludes = project.provider { -> project.ext.getCustomExcludes() }
}
}
}
}

tasks.register('check-extra-directories') {
List<Object> from = project.extensions.getByName('jib')['extraDirectories']['paths'].collect{ path -> path['from']}
List<Object> into = project.extensions.getByName('jib')['extraDirectories']['paths'].collect{ path -> path['into']}
List<Object> includes = project.extensions.getByName('jib')['extraDirectories']['paths'].collect{ path -> path['includes'].get()}
List<Object> excludes = project.extensions.getByName('jib')['extraDirectories']['paths'].collect{ path -> path['excludes'].get()}
println('extraDirectories (from): ' + from)
println('extraDirectories (into): ' + into)
println('extraDirectories (includes): ' + includes)
println('extraDirectories (excludes): ' + excludes)
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ project.ext.value = 'original'

project.afterEvaluate {
project.ext.value = 'updated'
project.ext.getCustomPermissions = { -> return ['/updated': '755'] }
}

jib {
Expand All @@ -33,9 +34,20 @@ jib {
]
}
}
extraDirectories {
paths = project.provider { ['src/main/' + project.ext.value + '-custom-extra-dir'] }
permissions = project.provider { -> project.ext.getCustomPermissions() }
}
}

tasks.register('showlabels') {
Map<String, String> prop = project.extensions.getByName('jib')['container']['labels'].get()
println('labels contain values ' + prop)
}

tasks.register('check-extra-directories') {
List<Object> paths = project.extensions.getByName('jib')['extraDirectories']['paths'].collect{ path -> path['from']}
Map<String, String> permissions = project.extensions.getByName('jib')['extraDirectories']['permissions'].get()
println('extraDirectories paths: ' + paths)
println('extraDirectories permissions: ' + permissions)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
updated
Copy link
Contributor

Choose a reason for hiding this comment

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

This content is not used for anything? If so, let's have it say "unused" instead of "updated", so people don't search for meaning when looking into this test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this file was added to commit its parent directory, so that it is an existing directory for the test project used by JibPluginTest. The file name and contents are trivial, I will change this to something less confusing.