jib icon indicating copy to clipboard operation
jib copied to clipboard

Use Gradle Property and Provider for creationTime and filesModificationTime

Open creckord opened this issue 3 years ago • 8 comments

Fixes #3708

Usage:

To use lazy evaluation for jib.container.creationTime or jib.container.filesModificationTime, the value can be provided as an ISO 8601 date-time string like this:

jib {
    container {
        creationTime = project.provider { value }
        filesModificationTime = project.provider { value }
    }
}

creckord avatar Jul 19 '22 14:07 creckord

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.

google-cla[bot] avatar Jul 19 '22 14:07 google-cla[bot]

Thank you @creckord! Please sign the CLA as described in the comment above so that we can review your contribution!

mpeddada1 avatar Jul 20 '22 14:07 mpeddada1

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 Property, we should be able to do away with setters. Also for reference: Lazy Properties section in official Gradle documentation which mentions Note that Gradle Groovy DSL generates setter methods for each Property-typed property in a task implementation

mpeddada1 avatar Aug 22 '22 14:08 mpeddada1

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:

String prop = project.extensions.jib.container.creationTime

Previously, this would have returned the actual property value. But since we return the property object now, Groovy's implicit type conversion with toString() leads to something like property(class java.lang.String, map(provider(?)))

The build file would need to be updated to use

String prop = project.extensions.jib.container.creationTime.get()

creckord avatar Sep 21 '22 13:09 creckord

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.

Awesome, thank you for the update and the additional tests! Looks like the coverage issue from earlier is all clear.

Note however, that this changes the result of something like this in the Gradle Groovy DSL:

String prop = project.extensions.jib.container.creationTime

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 .get() will be needed.

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.

emmileaf avatar Sep 21 '22 15:09 emmileaf

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.

creckord avatar Sep 21 '22 16:09 creckord