Use Gradle Property and Provider for creationTime and filesModificationTime#3709
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
SonarCloud Quality Gate failed. |
elefeint
left a comment
There was a problem hiding this comment.
Could you add unit tests for setCreationTime() / setFileModificationTime variations -- checking for nulls, checking that the default value is kept if not overridden. This will help with Sonar's line coverage requirement, too.
Also, could you update jib-gradle-plugin/README.md to reflect that the two properties can now accept providers?
jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/ContainerParameters.java
Outdated
Show resolved
Hide resolved
jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/ContainerParameters.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Regarding the setters (and possibly addressing the code coverage issue): would it make sense to remove them instead and have the getters set and expose the two Property<String>s, in a way that’s consistent with the approach from jib.container.label's lazy eval (#3242)? (See also #3242-comment and #3094-comment)
+1. Now that we are using |
…ib.container.filesModificationTime (GoogleContainerTools#3708)
56a4ae6 to
1a1f074
Compare
|
I've changed the getters to return the properties directly following this pattern, and removed the setters. I also added some more tests to check proper handling of defaults, nulls and finalized properties. Note however, that this changes the result of something like this in the Gradle Groovy DSL: Previously, this would have returned the actual property value. But since we return the property object now, Groovy's implicit type conversion with The build file would need to be updated to use |
…ers with Property (GoogleContainerTools#3708) * Removes String getters/setters * Adds getter for Property instead
3aa502c to
988352e
Compare
Awesome, thank you for the update and the additional tests! Looks like the coverage issue from earlier is all clear.
And yes you are right - following this pattern, when fetching this value in Groovy DSL like we do in the test build file, an additional Also, since the presubmit checks will require approval to re-run after new commits are pushed, feel free to drop a comment (if there is still work in-progress) when the latest commits are review-ready, and I'll re-trigger them. |
Thanks. I should have everything in now. |
emmileaf
left a comment
There was a problem hiding this comment.
Small typo nitpick, otherwise LGTM!
988352e to
c940921
Compare
|
Kudos, SonarCloud Quality Gate passed! |
elefeint
left a comment
There was a problem hiding this comment.
Thank you! We've discussed the changes to the user experience, and decided that those are worth it for the improvement.










Fixes #3708
Usage:
To use lazy evaluation for
jib.container.creationTimeorjib.container.filesModificationTime, the value can be provided as an ISO 8601 date-time string like this: