Configurable gradle configurations

[cromefire]

Fixes parts of #1778

This introduces a new configuration option configurationName that just sets the name of the gradle configuration that should be used.

[cromefire]

In practice this looks like:

jib {
    configurationName.set("docker")
}

[cromefire]

@googlebot I signed it!

[cromefire]

This is basically working, but I'm not (yet) enough into the mocking stuff to get the tests fixed up again

[cromefire]

Okay got all of them fixed.

[chanseokoh]

Thanks for the PR! I think overall this looks good but haven't looked into all the details or done tests. This week we're focused on another high priority work, so please be patient.

[chanseokoh]

Should we also update JibPlugin?

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

Lines 211 to 217 in 532a86b

	SourceSet mainSourceSet =
	project
	.getConvention()
	.getPlugin(JavaPluginConvention.class)
	.getSourceSets()
	.getByName(SourceSet.MAIN_SOURCE_SET_NAME);
	jibDependencies.add(mainSourceSet.getRuntimeClasspath());

[cromefire]

Yes, I think so, will have a look

[loosebazooka]

our of curiosity, can we just set the property to the system property value here?

public Property<String> getConfigurationName() {
  String property = System.getProperty(PropertyNames.CONFIGURATION_NAME);
  if (property != null) {
    configurationName.set(property);
  }
  return configurationName;
}

while this still looks a little odd, if it is possible, it matches more closely our previous patterns and it removes the ambiguous GetconfigurationName/readConfigurationName situation.

[cromefire]

I've realized it via a default value now, that sounds like a good idea anyway because it gives the user all the control. So if you want the override behavior you can just set the value to System.getProperty("some.property", "default"). The downside is of course that this is different from the rest of the API, creating an inconsistency in some cases.

[codecov]

Codecov Report

Merging #3034 (711e382) into master (d668d7c) will decrease coverage by 0.08%.
The diff coverage is 35.00%.

@@             Coverage Diff              @@
##             master    #3034      +/-   ##
============================================
- Coverage     71.18%   71.09%   -0.09%     
+ Complexity     2314     2313       -1     
============================================
  Files           278      278              
  Lines          9761     9784      +23     
  Branches        990      991       +1     
============================================
+ Hits           6948     6956       +8     
- Misses         2471     2483      +12     
- Partials        342      345       +3     

Impacted Files	Coverage Δ	Complexity Δ
...google/cloud/tools/jib/gradle/BuildDockerTask.java	4.68% <0.00%> (-0.24%)	2.00 <0.00> (ø)
.../google/cloud/tools/jib/gradle/BuildImageTask.java	5.00% <0.00%> (-0.27%)	2.00 <0.00> (ø)
...om/google/cloud/tools/jib/gradle/BuildTarTask.java	4.47% <0.00%> (-0.29%)	2.00 <0.00> (ø)
...e/cloud/tools/jib/gradle/skaffold/FilesTaskV2.java	0.00% <0.00%> (ø)	0.00 <0.00> (ø)
...e/cloud/tools/jib/gradle/skaffold/SyncMapTask.java	0.00% <0.00%> (ø)	0.00 <0.00> (ø)
...om/google/cloud/tools/jib/gradle/JibExtension.java	82.45% <71.42%> (-1.55%)	19.00 <1.00> (+1.00)	⬇️
...loud/tools/jib/gradle/GradleProjectProperties.java	65.21% <75.00%> (+0.31%)	31.00 <0.00> (-2.00)	⬆️
...a/com/google/cloud/tools/jib/gradle/JibPlugin.java	83.33% <100.00%> (+0.57%)	17.00 <0.00> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d668d7c...711e382. Read the comment docs.

[cromefire]

Should we also update JibPlugin?

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

Lines 211 to 217 in 532a86b

	SourceSet mainSourceSet =
	project
	.getConvention()
	.getPlugin(JavaPluginConvention.class)
	.getSourceSets()
	.getByName(SourceSet.MAIN_SOURCE_SET_NAME);
	jibDependencies.add(mainSourceSet.getRuntimeClasspath());

That breaks the build (seems to exclude the own code) and it works without it, so I guess not (I just added it, instead of replacing it, that should do the job).

[cromefire]

Not sure what's up with the code coverage

[chanseokoh]

LGTM. Thanks for your contribution! I think we can merge this when ready.

[cromefire]

Will also submit a quick followup (after merge), to get the gradle version up to date, it's basically a 2 line change now with the new testing

[chanseokoh]

Merging now. Thanks again for your contribution!

@mpeddada1 we need to add the config param to the plugin doc and also update CHANGELOG.

[chanseokoh]

@cromefire I was trying to test the way we support this by creating a small Gradle sample (the following code).

public class SimpleTask extends DefaultTask {

  private final Property<String> contents;

  @Inject
  public SimpleTask(ObjectFactory objects) {
    contents = objects.property(String.class).convention("default");
  }

  @Input
  public Property<String> getContents() {
    String property = System.getProperty("simpleTask.contents");
    if (property != null) {
      contents.set(property);
    }
    return contents;
  }

  @OutputFile
  public File getOutputFile() {
    return getProject().getBuildDir().toPath().resolve("simpleTask.out").toFile();
  }

  @TaskAction
  public void doWork() {
     ... calls getContents() ...
  }

It fails with an error when I provide the system property simpleTask.contents on the command-line.

The value for task ':myTask' property 'contents' is final and cannot be changed any further.

I wasn't able to reproduce this in Jib, but I'm concerned that the way we implemented this has the potential to fail if conditions are met. Your thoughts?

[cromefire]

Maybe it's because of your usage of @Inject?

[chanseokoh]

I think @Inject is necessary, and I am not sure if the constructor is related to the error.

> Could not create task ':myTask'.
   > Could not create task of type 'SimpleTask'.
      > The constructor for type SimpleTask should be annotated with @Inject.

The error is due to calling contents.set(property) when the contents is already finalized. Maybe the code like this is not really safe? 😞

[cromefire]

Maybe one should check for property != null && !property.equals(contents.get()), that should avoid that problem

[cromefire]

All in all it is a work around and not really how it was intended to be used so yeah that's the tradeoff...

[chanseokoh]

Maybe one should check for property != null && !property.equals(contents.get()), that should avoid that problem

Oh, that did the trick! I now see it was applying the system property multiple times. Thanks! I should update the code.

[chanseokoh]

FTR, this can be problematic and should be fixed. #3034 (comment)