Use Gradle Property and Provider to enable lazy evaluation for jib.to.image and jib.to.tags by mpeddada1 · Pull Request #2739 · GoogleContainerTools/jib

mpeddada1 · 2020-09-03T00:05:10Z

Fixes #2727

Usage:

If user wants to use lazy evaluation for jib.to.image and jib.to.tags with String value they can do the following:
jib.to.image = project.provider{value} or jib.to.tags = project.provider{[value, "tag2"]}

chanseokoh

Aside from writing unit tests, have you manually tested if this works?

jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/TargetImageParameters.java

mpeddada1 · 2020-09-03T17:01:47Z

Aside from writing unit tests, have you manually tested if this works?
I'm still manually experimenting with an example build.gradle to see if it actually works so this PR is still a work in progress.

chanseokoh

Just went over it briefly as a draft.

jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/TargetImageParameters.java

chanseokoh · 2020-09-03T20:10:28Z

jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/TargetImageParameters.java

+  }
+
+  public void setImage(Provider<String> image) {
+    if (image != null) {


I speculate that we won't need this null check.

hm removing this check is resulting in an UnexpectedBuildFailure with the message, Cannot set the value of a property using a null provider in places where the target image value is not set. (For example,

jib/jib-gradle-plugin/src/test/java/com/google/cloud/tools/jib/gradle/skaffold/FilesTaskV2Test.java

Line 110 in f892bdb

new SkaffoldFilesOutput(verifyTaskSuccess(multiTestProject, "simple-service"));

Aha, looks like Gradle prefers this method signature over String. If that's the case, I think it should be

Suggested change

if (image != null) {

if (image == null) {

this.image = null;

} else {

this.image.set(image);

}

Ah, exactly! You're right, when jib.to.image is set to null then Gradle prefers this method. With the if (image != null) checker, we are currently returning the message Missing target image parameter, perhaps you should add a 'jib.to.image' configuration parameter to your build.gradle... if jib.to.image is not specified, which seems like expected behavior. I could be missing something here but why do we want to make the Property be null instead of not just not setting a value for it?

Good question.

From the general API perspective, irrelevant of Jib: let's say a Java developer is using some Java library which has Person.setEmail(String). It's an optional property, and by default getEmail() returns null. Let's say the developer does

// prints out "no email given" by default System.out.println(person.getEmail() == null ? "no email given" : person.getEmail()); person.setEmail("foo@example.com"); person.setEmail(null); System.out.println(person.getEmail() == null ? "no email given" : person.getEmail());

Then, which would be reasonable for the last println() to print: no email given or foo@example.com? Of course, it may depend on the situation, but in this case, I expect setEmail(null) to clear the previous value and make the property "missing"; it is counter-intuitive it becomes a no-op.

Also note that jib.to.image is basically an optional property (as indicated by the @Optional annotation). Of course, it is required when pushing to a remote registry (jib:build), and it will say "Missing target image parameter." However, it isn't required when pushing to a local Docker daemon (jib:dockerBuild); in this case, Jib will auto-pick some name for it.

But, before we add the null check, I'd like to verify Gradle always calls setImage(null) even when jib.to.image is simply missing (not configured) rather than when jib.to.image is explicitly set to null. Reading https://github.com/GoogleContainerTools/jib/pull/2739/files#r485907745, it looks like this is only when explicitly setting jib.to.image to null. In that case, I don't think we need the null check actually; I think we can just fix the failing test.

Thank you so much for the detailed explanation!

And with regards to the last point about https://github.com/GoogleContainerTools/jib/pull/2739/files#r485907745, the issue only occurs when jib.to.image is explicitly set to null. I believe the tests that were failing set jib.to.image to System.getProperty("_TARGET_IMAGE") and didn't initialize _TARGET_IMAGE? Because System.getProperty(String key) returns null if there is no property with that key.

Yeah, that makes sense. I think we don't actually need a null check; we can just fix the test, e.g., setting _TARGET_IMAGE to "ignored."

jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/TargetImageParameters.java

chanseokoh · 2020-09-09T16:24:14Z

jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/TargetImageParameters.java

+  }
+
+  public void setImage(Provider<String> image) {
+    if (image != null) {


Aha, looks like Gradle prefers this method signature over String. If that's the case, I think it should be

Suggested change

if (image != null) {

if (image == null) {

this.image = null;

} else {

this.image.set(image);

}

chanseokoh · 2020-09-09T16:25:43Z

jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/TargetImageParameters.java

+  }
+
+  public void setTags(Provider<Set<String>> tags) {
+    this.tags.set(tags);


In light of https://github.com/GoogleContainerTools/jib/pull/2739/files#r485008215, I wonder if we need a null check. Can you run a manual test and see which of the two setTags() gets called?

That is a good idea. So unlike the case with the image, setting jib.to.tags=null is resulting in GroovyRuntimeException with Ambiguous method overloading...Cannot resolve which method to invoke for [null] due to overlapping prototypes between where one setTags is not preferred over the other. The issue occurs when only setTags(Provider<Set<String>>) is present, which is when setting jib.to.tags=null results in the Cannot set the value of property using null provider message.

Passing in a set or provider<set> that contains null seems to resulting in NPE so I think we need to handle that?

Hmm... I think we should be fine as long as nothing breaks when jib.to.tags is missing (not configured). People shouldn't explicitly set it to null anyway. That said, I now wonder the test failure you mentioned in https://github.com/GoogleContainerTools/jib/pull/2739/files#r485008215 is when you explicitly set the property to null rather than when it's missing. If former, I don't think we need the null check there either.

Passing in a set or provider that contains null seems to resulting in NPE so I think we need to handle that?

Let's file an issue for that. It basically sounds like an orthogonal issue we don't want to mix in here.

Right that's a good point. I filed an issue for it in #2760

jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/TargetImageParameters.java

chanseokoh

Looks good to me. I'll let @loosebazooka approve this.

jib-gradle-plugin/src/test/java/com/google/cloud/tools/jib/gradle/skaffold/FilesTaskV2Test.java

jib-gradle-plugin/src/test/java/com/google/cloud/tools/jib/gradle/skaffold/SyncMapTaskTest.java

chanseokoh · 2020-09-10T21:00:42Z

If user wants to use lazy evaluation for jib.to.image and jib.to.tags with String value they can do the following:
jib.to.image = project.provider{value} or jib.to.tags = project.provider{[value, "tag2"]}

@loosebazooka so we decided to accept Provider<String>, but I guess Supplier<String> will similarly work, which will enable syntax like jib.to.image = () -> "value". I just thought using Provider was like a convention, but I'm actually curious why we decided to use Provider and not something like Supplier.

loosebazooka · 2020-09-14T17:38:00Z

...in/src/test/resources/gradle/projects/lazy-evaluation/src/main/java/com/test/HelloWorld.java

+public class HelloWorld {
+
+  public static void main(String[] args) throws URISyntaxException, IOException {
+    try (Reader reader =


If you're not using the contents of this file to do anything special, just a simple System.out.println("Hello world") is probably sufficient.

loosebazooka · 2020-09-14T17:40:41Z

jib-gradle-plugin/src/test/java/com/google/cloud/tools/jib/gradle/JibPluginTest.java

+  @Test
+  public void testLazyEvalForImageAndTags() {
+    try {
+      testProject.build(JibPlugin.JIB_EXTENSION_NAME);


I think this technically works, but is confusing. Maybe JibPlugin.BUILD_IMAGE_TASK_NAME is more appropriate?

Ah that makes more sense

loosebazooka · 2020-09-14T17:41:56Z

jib-gradle-plugin/src/test/java/com/google/cloud/tools/jib/gradle/JibPluginTest.java

+      String output = ex.getBuildResult().getOutput().trim();
+
+      // Regex to parse through image and tag values from build output.
+      String cyanWordRegex = "\\u001B\\[36m(.+?)\\u001B\\[0m";


I don't know if this is true, but if you use -Djib.console=plain, it might be easier to read the output, and you can ignore the cyan coloring and stuff?

Also since we already know the image name and tags, could we just use those in the comparison directly instead of extracting them out and then using the Assert at line 404, 405?

Thanks for the suggestion! Unfortunately, jib.console=plain doesn't display the progress bar but still keeps the cyan coloring:(

Also since we already know the image name and tags, could we just use those in the comparison directly instead of extracting them out and then using the Assert at line 404, 405?
Good point, I just applied the change.

How about passing --console=plain to Gradle? (Note it is not -D... this time.)

Hm the cyan coloring still appears when --console=plain is passed to Gradle and when Djib.console=plain is used.

Ugh, you're right. I see there are a couple things that are always colored.

Containerizing application to Docker daemon as francium25/test...

Built image to Docker daemon as francium25/test

I think we should fix this. Could you file a bug (and also link this issue to get rid of the interim code to handle color)?

Yup, that's a good idea. I added a TODO with reference to issue #2764.

…-prop

jib-gradle-plugin/src/test/java/com/google/cloud/tools/jib/gradle/JibPluginTest.java

loosebazooka · 2020-09-16T14:31:20Z

@loosebazooka so we decided to accept Provider, but I guess Supplier will similarly work, which will enable syntax like jib.to.image = () -> "value". I just thought using Provider was like a convention, but I'm actually curious why we decided to use Provider and not something like Supplier.

Well the underlying type is Property which can potentially accept either a value or Property, Provider by nature appears to be immutable, so I don't think the base type should not be Provider for user configured objects (but I think this is only enforced by convention or by using final).

~~As for Supplier, I'm not sure, I can't find any docs on it at the moment and it appears to be an internal object?~~
I realize now you mean Java Supplier, I think the Property interface is built to accept both provider and value object. Maybe gradle created Provider before moving to Java8?

* Add setTag method that accepts a list

Using gradle Property and Provider to enable lazy evaluation

6f4a719

googlebot added the cla: yes label Sep 3, 2020

chanseokoh reviewed Sep 3, 2020

View reviewed changes

fixes

934d9b0

mpeddada1 added 3 commits September 3, 2020 15:40

Add lazy eval for jib.to.tags

58575be

remove unnecessary changes

2db50d0

remove more unnecessary changes

02ca7b9

mpeddada1 changed the title ~~Using Gradle Property and Provider to enable lazy evaluation~~ Using Gradle Property and Provider to enable lazy evaluation for jib.to.image and jib.to.tags Sep 3, 2020

formatting

6d7b6ea

mpeddada1 changed the title ~~Using Gradle Property and Provider to enable lazy evaluation for jib.to.image and jib.to.tags~~ Use Gradle Property and Provider to enable lazy evaluation for jib.to.image and jib.to.tags Sep 3, 2020

chanseokoh reviewed Sep 3, 2020

View reviewed changes

addressing comment suggestions

ae79d46

chanseokoh reviewed Sep 9, 2020

View reviewed changes

removing null checker

fdfad6d

chanseokoh reviewed Sep 10, 2020

View reviewed changes

jib-gradle-plugin/src/test/java/com/google/cloud/tools/jib/gradle/skaffold/FilesTaskV2Test.java Outdated Show resolved Hide resolved

jib-gradle-plugin/src/test/java/com/google/cloud/tools/jib/gradle/skaffold/SyncMapTaskTest.java Outdated Show resolved Hide resolved

mpeddada1 added 3 commits September 11, 2020 18:12

adding tests and sample project

6837d62

adding tests and fixing file naming

27c3431

more fixes

c8ccb15

loosebazooka reviewed Sep 14, 2020

View reviewed changes

mpeddada1 added 3 commits September 14, 2020 17:21

addressing comments

301fc9d

Merge branch 'master' of github.com:GoogleContainerTools/jib into git…

c04f1cb

…-prop

Modifying CHANGELOG

1731a70

loosebazooka reviewed Sep 15, 2020

View reviewed changes

jib-gradle-plugin/src/test/java/com/google/cloud/tools/jib/gradle/JibPluginTest.java Show resolved Hide resolved

mpeddada1 added 3 commits September 15, 2020 16:13

test clean output

226ee62

remove extra space

a238e66

Use MatcherAssert instead of regexes

87ba48e

mpeddada1 marked this pull request as ready for review September 16, 2020 16:02

chanseokoh mentioned this pull request Sep 16, 2020

Disable cyan coloring in build output when --console=plain is used. #2764

Closed

Added TODO to remove cyan color filtering once #2764 is submitted.

4378bd3

loosebazooka approved these changes Sep 18, 2020

View reviewed changes

mpeddada1 merged commit faff19b into master Sep 18, 2020

mpeddada1 deleted the git-prop branch September 18, 2020 14:27

mpeddada1 mentioned this pull request Sep 18, 2020

Fix to handle integration test failure caused by #2739 #2773

Merged

mpeddada1 added a commit that referenced this pull request Sep 18, 2020

Fix to handle integration test failure caused by #2739 (#2773)

23defe8

* Add setTag method that accepts a list

This was referenced Nov 30, 2020

JIB to.auth.password should be a Property, not a plain String dubinsky/cloud-run#6

Closed

Convert jib.to.auth.username and jib.to.auth.password to properties #2905

Closed

emmileaf mentioned this pull request Aug 11, 2022

Gradle: Support lazy configuration of properties, specifically jib.to.image. #3489

Closed

-    if (image != null) {
+    if (image == null) {
+      this.image = null;
+    } else {
+      this.image.set(image);
+    }

Conversation

mpeddada1 commented Sep 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chanseokoh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mpeddada1 commented Sep 3, 2020

Uh oh!

chanseokoh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chanseokoh Sep 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mpeddada1 Sep 10, 2020 • edited by chanseokoh Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mpeddada1 Sep 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

chanseokoh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

chanseokoh commented Sep 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loosebazooka Sep 14, 2020 • edited by chanseokoh Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mpeddada1 commented Sep 3, 2020 •

edited

Loading

chanseokoh Sep 9, 2020 •

edited

Loading

mpeddada1 Sep 10, 2020 •

edited by chanseokoh

Loading

mpeddada1 Sep 9, 2020 •

edited

Loading

loosebazooka Sep 14, 2020 •

edited by chanseokoh

Loading

loosebazooka commented Sep 16, 2020 •

edited

Loading