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 for creationTime and filesModificationTime #3709

Merged

Conversation

creckord
Copy link
Contributor

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 }
    }
}

@google-cla
Copy link

google-cla bot commented Jul 19, 2022

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.

@mpeddada1
Copy link
Contributor

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

@sonarcloud
Copy link

sonarcloud bot commented Jul 25, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

62.5% 62.5% Coverage
0.0% 0.0% Duplication

Copy link
Contributor

@elefeint elefeint left a comment

Choose a reason for hiding this comment

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

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?

@elefeint elefeint requested a review from emmileaf August 11, 2022 22:32
Copy link
Contributor

@emmileaf emmileaf left a comment

Choose a reason for hiding this comment

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

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)

@mpeddada1
Copy link
Contributor

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

@creckord creckord force-pushed the 3708-creationtime-property branch 2 times, most recently from 56a4ae6 to 1a1f074 Compare September 21, 2022 13:24
@creckord
Copy link
Contributor Author

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 creckord requested review from elefeint and removed request for mpeddada1 September 21, 2022 14:41
…ers with Property (GoogleContainerTools#3708)

* Removes String getters/setters
* Adds getter for Property instead
@emmileaf
Copy link
Contributor

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.

@creckord
Copy link
Contributor Author

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.

Copy link
Contributor

@emmileaf emmileaf left a comment

Choose a reason for hiding this comment

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

Small typo nitpick, otherwise LGTM!

jib-gradle-plugin/README.md Outdated Show resolved Hide resolved
@sonarcloud
Copy link

sonarcloud bot commented Sep 22, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

100.0% 100.0% Coverage
0.0% 0.0% Duplication

Copy link
Contributor

@elefeint elefeint left a comment

Choose a reason for hiding this comment

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

Thank you! We've discussed the changes to the user experience, and decided that those are worth it for the improvement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable lazy evaluation of jib.container.creationTime and jib.container.filesModificationTime
5 participants