-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Conversation
99faaa0
to
b79d5ec
Compare
Hmm I’m not sure why codecov is marking the overloaded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great to me.
Could you update documentation in jib-gradle-plugin/README.md
?
return ConfigurationPropertyValidator.parseMapProperty(property); | ||
Map<String, String> parsedPermissions = | ||
ConfigurationPropertyValidator.parseMapProperty(property); | ||
if (!parsedPermissions.equals(permissions.get())) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -0,0 +1 @@ | |||
updated |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@elefeint Thank you for the review! I’ve updated the readme and made a note in the changelog. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the really detailed PR description! This is a pretty complex lazy evaluation use case to implement!
...radle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/ExtraDirectoriesParameters.java
Outdated
Show resolved
Hide resolved
...radle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/ExtraDirectoriesParameters.java
Outdated
Show resolved
Hide resolved
...radle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/ExtraDirectoriesParameters.java
Show resolved
Hide resolved
537e008
to
11d9dd1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great! Added a couple of final minor comments.
jib-gradle-plugin/src/test/java/com/google/cloud/tools/jib/gradle/JibExtensionTest.java
Outdated
Show resolved
Hide resolved
jib-gradle-plugin/src/test/java/com/google/cloud/tools/jib/gradle/JibExtensionTest.java
Outdated
Show resolved
Hide resolved
.../src/test/resources/gradle/projects/lazy-evaluation/src/main/updated-custom-extra-dir/unused
Outdated
Show resolved
Hide resolved
jib-gradle-plugin/src/test/java/com/google/cloud/tools/jib/gradle/JibPluginTest.java
Outdated
Show resolved
Hide resolved
jib-gradle-plugin/src/test/java/com/google/cloud/tools/jib/gradle/JibPluginTest.java
Outdated
Show resolved
Hide resolved
...radle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/ExtraDirectoriesParameters.java
Outdated
Show resolved
Hide resolved
Kudos, SonarCloud Quality Gate passed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent! Looks good on my end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice!
In this PR:
Enable lazy evaluation in Gradle for various
jib.extraDirectories
parameters (ref: discussions in #2732).jib.extraDirectories.permissions
:Map<String, String>
=>MapProperty<String, String>
jib.extraDirectories.paths.path.from
:Path
=>Property<Path>
jib.extraDirectories.paths.path.into
:String
=>Property<String>
Lazy evaluation (via a Provider) for configuring the
jib.extraDirectories.paths
parameter directly is also enabled. Previously aProvider
was able to be passed, but gets treated as a non-provider object and resolves file paths too early.Both use cases below are supported in this PR: