Share PluginConfigurationProcessor between Gradle and Maven #1163

chanseokoh · 2018-10-18T22:21:07Z

This removes plugin-specific PluginConfigurationProcessor classes by merging them in the new PluginConfigurationProcessor in jib-plugins-common.

Also:
Fixes #1090
Closes #1088

~~Did not touch tests, so builds will fail. I'm setting up the PR to get the early, high-level feedback if this direction seems good.~~

I tried to share the PluginConfigurationProcessor code, and it turns out to be a no easy task. ~~The old code cannot be removed for now.~~

By the way, my assumption: PluginConfigurationProcessor is the class for reading and interpreting raw config parameters to instantiate and hold a JibContainerBuilder instance. The longest processCommonConfiguration() I think is mainly for reusing code between jib:Build, jib:DockerBuild, and jib:BuildTar for configuring JibContainerBuilder. The class also has grown to add a couple more static utility methods to share code between tasks.

In order to share code, I think we need an adapter for retrieving raw configuration values from Maven and Gradle, so for now, I created the interface RawConfigurations, which is mostly a POJO mapping Maven and Gradle raw configs.

~~The PR may have some pitfalls; for now, this PR is for early feedback.~~

…igurationProcessor

coollog · 2018-10-19T13:54:43Z

The RawConfiguration is definitely an improvement. I think we can structure this even more to better organize the common logic, in something like the following:

Maven has JibPluginConfiguration and Gradle has JibExtension that are the plugin-specific configuration.
Validate JibPluginConfiguration and JibExtension at the plugin-configuration level -> generates MavenRawConfiguration and GradleRawConfiguration.
Canonicalize the RawConfiguration into a CanonicalConfiguration (this will probably be the bulk of the code)
Validate the CanonicalConfiguration into a ValidatedConfiguration
Set up and execute the JibContainerBuilder.

Essentially, we want to try to:

Have a canonicalized configuration as early in the process as possible so that we can share all code after canonicalization, and
Validate and report errors as early in the process as possible and keep validation/error-reporting together rather than dispersed.

Let's discuss this a bit more.

chanseokoh · 2018-10-19T14:43:45Z

Sounds good at the high level.

Validate JibPluginConfiguration and JibExtension at the plugin-configuration level -> generates MavenRawConfiguration and GradleRawConfiguration.

Validate the CanonicalConfiguration into a ValidatedConfiguration

What's the difference of the two validations? Will the first validation require our own validation logic? If so, shouldn't such code be shared too?

Have a canonicalized configuration as early in the process as possible

And what is the exact definition of CanonicalConfiguration? (I think there is no confusion about RawConfiguration; it is the as-is parameter values.) I noticed a few things to consider in this process:

Some tasks do not require all the config parameters or uber validation. For example, jib:dockerBuild will succeed even if you have a malformed target image reference. Another example is jib:exportDockerContext, which doesn't require auth at all.
It might actually make sense to delay validation. For example, decrypting Maven settings.xml password may fail. However, decrypting the password for the target registry is needed only for the jib:build goal. Currently, we don't fail even if there is an error in settings.xml in this case.

BTW, for now I don't see a need to have a separate class for ValidatedConfiguration. Just validating values of CanonicalConfiguration seems enough, but we will see. (I think I'm not a fan of having another class just for the sake of type checking for the same semantic object.) For example, it sounds like I have a Car class and then a GoodCar class.

coollog · 2018-10-19T17:03:02Z

What's the difference of the two validations? Will the first validation require our own validation logic? If so, shouldn't such code be shared too?

Yea, I was mostly just suggesting a potential pipeline, but the first validation is for validating at the configuration level for cases not handled by the built-in plugin validators (like @Required). The second one is validating for the parameters for canonicalization (like erroring if the configured credential helper doesn't exist).

And what is the exact definition of CanonicalConfiguration? (I think there is no confusion about RawConfiguration; it is the as-is parameter values.) I noticed a few things to consider in this process:

RawConfiguration will be directly the fields configurable on the plugins, whereas CanonicalConfiguration will be the fields to configure the JibContainerBuilder, which include, for example, the layer configurations to set.

It might actually make sense to delay validation. For example, decrypting Maven settings.xml password may fail. However, decrypting the password for the target registry is needed only for the jib:build goal. Currently, we don't fail even if there is an error in settings.xml in this case.

Yes, I think these errors will be more for execution errors rather than configuration validation errors.

BTW, for now I don't see a need to have a separate class for ValidatedConfiguration. Just validating values of CanonicalConfiguration seems enough, but we will see.

I think these are mostly just to indicate whether they are validated or not. It can be the same class that implements CanonicalConfiguration and ValidatedConfiguration, with perhaps ValidatedConfiguration exposing methods that can only be called after validation.

chanseokoh · 2018-10-19T17:22:29Z

Okay, for now I think we can move forward with RawConfiguration primarily for sharing the bulk of PluginConfigurationProcessor. The PluginConfigurationProcessor is tightly coupled with JibContainerBuilder (a PluginConfigurationProcessor instance is actually a holder for JibContainerBuilder), so just trying to share the code would be the first step for such a config processing pipeline. (Not that I intend to complete the whole pipeline anytime soon.)

chanseokoh · 2018-10-19T17:34:47Z

RawConfiguration will be directly the fields configurable on the plugins, whereas CanonicalConfiguration will be the fields to configure the JibContainerBuilder, which include, for example, the layer configurations to set.

Oh, forgot to say this. We may run into some chick-and-egg type of complication in this regard. For example, to compute the layer configurations to set, we need to know the right container.appRoot to use, which is not the raw configuration value (e.g., the default appRoot differs depending on WAR vs. non-WAR). That is, the canonical configuration values of appRoot depend on the project info, but currently, the class that abstracts and represents project info (GradleProjectProperties) requires validated canonical configuration. We can think about this later.

coollog · 2018-10-19T17:37:14Z

Yes, that sounds good. We can definitely work towards more structured layouts in an iterative manner.

chanseokoh · 2018-10-19T20:51:39Z

Will try to make this PR work. Tagging the "PR: Not Ready" label until then.

chanseokoh · 2018-10-23T16:32:56Z

Ready for review. For reviewers, here are some core changes:

Removed the two PluginConfigurationProcessor (PCP) classes from Maven and Gradle and migrate the code to the new PluginConfigurationProcessor in plugins-common.
Created RawConfiguration for providing raw, unprocessed config values in order to share PluginConfigurationProcessor. MavenRawConfiguration and GradleRawConfiguration acts as adapters on top of plugin-specific config parameters. Added Tests.
Fixes Be consistent with evaluating Maven and Gradle parameter values #1090 by consistently computes the base image and the app root. (Gradle doesn't update the raw config values after project evaluation.)
PCP tests have been merged and migrated too.
PCP.disableHttpLogging() could not be migrated. Created TaskCommon (Gradle) and MojoCommon (Maven) for such methods.
getAppRootChecked() is migrated, but there is some chicken-and-egg complication with the current design, so for now, the original getAppRootChecked() is duplicated and placed onto TaskCommon and MojoCommon.

chanseokoh · 2018-10-23T16:33:33Z

Travis is stalling (also in other PRs), so let's ignore that for now.

TadCordle · 2018-10-23T20:08:40Z

I think I'm a bit late to the "high-level feedback" stage, but I wanted to give my 2 cents. Correct me if I'm wrong (I very well might be, I only skimmed a bit of this PR), but now the available configuration parameters are referenced in RawConfiguration, MavenRawConfiguration, GradleRawConfiguration, JibExtension, JibPluginConfiguration, BuildConfiguration, and JibContainerBuilder? Plus if we ever go down the CanonicalConfiguration and ValidatedConfiguration route, that's two more that will be containing essentially the same information as the other 7 files? It'd get to the point where we have to update 9 files just to add one new parameter before even adding the new parameter's functionality, which just doesn't sound straightforward to maintain (it's already getting bad enough in code having to pass new configuration parameters through plugin configurations -> JibContainerBuilder -> BuildImageStep -> Image -> ImageToJsonTranslator/JsonToImageTranslator -> ContainerConfigurationTemplate for certain things). Some of this is unavoidable since we can't consolidate the Maven/Gradle specific stuff, but to me a perfect world is 1 gradle configuration, 1 maven configuration, and 1 non-overlapping set of core configurations.

I can see the positives of having a structured pipeline for canonicalization and validation, but this is just my opinion from a maintenance/code simplicity perspective. We can move on with this if others think it's a good idea, but I think it's something worth thinking about.

chanseokoh · 2018-10-23T21:11:08Z

I see what you mean, but I think it isn't like we are making it worse. Let's look into the classes you mentioned.

JibExtension and JibPluginConfiguration: They are plugin-specific models given from the beginning. Basically we won't ever call getters directly. RawConfiguration will be the frontend for them to get the raw values, and any code should use RawConfiguration and forget about JibExtension or JibPluginConfiguration to retrieve values.
RawConfiguration: An interface needed to abstract away plugin-specific code, so I think this is a necessary one to share code that processes raw config values to compute effective end config parameters. MavenRawConfiguration, and GradleRawConfiguration just implements the interface, wrapping plugin-specific models (hence very thin). I think it may even be possible to make JibExtension and JibPluginConfiguration implement the interface directly, although not sure if it is a good idea or even possible. If we can do that, we can just remove MavenRawConfiguration and GradleRawConfiguration. For now, it seems OK to let JibExtension be just the direct model for Gradle config parameters.
BuildConfiguration: this is actually CanonicalConfiguration, from my understanding. The effective end config parameter values that should be used in actuality (e.g., correct base image based on WAR or non-WAR).
ValidatedConfiguration: as I said previously (Share PluginConfigurationProcessor between Gradle and Maven #1163 (comment)), I don't fully agree at the moment this class is needed. In any case, I don't think we'll set up the pipeline anytime soon.

chanseokoh · 2018-10-23T21:16:02Z

Oh, and for JibContainerBuilder, currently it looks like most of the work it does is to instantiate BuildConfiguration or ContainerConfiguration. In the end, I think this work should be done by the entity that processes the raw configuration to produce a canonical configuration (BuildConfiguration).

chanseokoh · 2018-10-23T21:21:37Z

@TadCordle does that make sense or do you still have something to add?

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

...on/src/main/java/com/google/cloud/tools/jib/plugins/common/PluginConfigurationProcessor.java

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

coollog · 2018-10-23T23:30:30Z

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

+      EventDispatcher eventDispatcher =
+          new DefaultEventDispatcher(projectProperties.getEventHandlers());
+      ImageReference targetImageReference =
+          ConfigurationPropertyValidator.getGeneratedTargetDockerTag(


nit: Doesn't have to be this PR but noticed this method is rather long with embedded logic - we probably should break this up a bit.

Yeah, not just BuildDockerTask. BuildImageTask and BuildTarTask are equally long and we can certainly do better.

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

...plugins-common/src/main/java/com/google/cloud/tools/jib/plugins/common/RawConfiguration.java

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

coollog · 2018-10-23T23:54:16Z

...plugins-common/src/main/java/com/google/cloud/tools/jib/plugins/common/RawConfiguration.java

+  boolean getUseOnlyProjectCache();
+
+  @Nullable
+  AuthProperty getInferredAuth(String authTarget) throws InferredAuthRetrievalException;


Hmm, it feels a bit off that this is returning an AuthProperty here given that this is the raw configuration meant as the common configuration to pipe the maven/gradle-specific configuration into, but here it feels like it is looping back to plugin configuration since AuthProperty is implemented by the maven configuration JibPluginConfiguration.AuthConfiguration.

AuthProperty is merely an interface in itself that supplies auth info: username, password, and the source. I guess you feel a bit off here because you are associating the interface tightly with a certain implementation (which happens to be JibPluginConfiguration.AuthConfiguration in this case) or this AuthProperty interface is exclusively created for plugin auth configuration usage. But from my perspective, AuthProperty just provides what the interface describes: username, password, and the source. So I don't feel it's looping back; the raw configuration is providing auth info, and this is simply done through some reasonable interface that is reusable across the codebase. It's just that AuthConfiguration is one implementation that is Maven specific. Does that make sense?

Okay, we can keep it as this. It might just feel off to me since the getInferredAuth is the only method here that performs a costly operation and is only implemented on the Maven side. Maybe it belongs in the processor class to process it and feed it into the configuration rather than be a method on the raw configuration, but this is fine for now.

Yeah, it also makes sense to feed it to the config processor rather than having it in the raw configuration, and I have thought about that too, but after some thinking, I got to the conclusion that this auth info is just values from a static file (settings.xml) not much different from getting values from pom.xml, and conceptually fits with "raw configuration values". However, I also see we may need to decrypt the values, so in another sense, it requires some processing. I'll add a TODO for considering passing an additional base image auth provider into the config processor.

jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/JibPluginConfiguration.java

TadCordle · 2018-10-24T18:39:51Z

@chanseokoh Yeah that makes sense. Just seems like a lot of layers to me.

…igurationProcessor

coollog · 2018-10-26T02:15:22Z

...on/src/main/java/com/google/cloud/tools/jib/plugins/common/NotAbsoluteUnixPathException.java

+/** Indicates that the string path is not in the absolute unix-path style. */
+public class NotAbsoluteUnixPathException extends Exception {
+
+  public NotAbsoluteUnixPathException(String appRoot, Throwable ex) {


Probably should use another var name than appRoot

...on/src/main/java/com/google/cloud/tools/jib/plugins/common/PluginConfigurationProcessor.java

coollog · 2018-10-26T02:25:51Z

...plugins-common/src/main/java/com/google/cloud/tools/jib/plugins/common/RawConfiguration.java

+  boolean getUseOnlyProjectCache();
+
+  @Nullable
+  AuthProperty getInferredAuth(String authTarget) throws InferredAuthRetrievalException;


Okay, we can keep it as this. It might just feel off to me since the getInferredAuth is the only method here that performs a costly operation and is only implemented on the Maven side. Maybe it belongs in the processor class to process it and feed it into the configuration rather than be a method on the raw configuration, but this is fine for now.

coollog

LGTM. Will have a second reviewer approve.

coollog · 2018-10-26T16:15:20Z

...-common/src/main/java/com/google/cloud/tools/jib/plugins/common/AppRootInvalidException.java

+ */
+public class AppRootInvalidException extends Exception {
+
+  public AppRootInvalidException(String pathValue, Throwable ex) {


nit: I think the pathValue (maybe call it invalidAppRoot?) should be its own field with a getter rather than the message of the exception?

Will do. I think we still need to set the exception message to the pathValue though.

...on/src/main/java/com/google/cloud/tools/jib/plugins/common/PluginConfigurationProcessor.java

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

jib-maven-plugin/src/test/java/com/google/cloud/tools/jib/maven/MavenRawConfigurationTest.java

…igurationProcessor

chanseokoh added 6 commits October 18, 2018 12:43

Remove obsolete code

90b0bc7

Refactor PluginConfigurationProcessor

6fb646d

Merge remote-tracking branch 'origin/master' into refactor-PluginConf…

17bd694

…igurationProcessor

wip

02fa8c9

wip

aed14de

Add TODOs

69aa98a

googlebot added the cla: yes label Oct 18, 2018

coollog added the in progress label Oct 18, 2018

chanseokoh added 4 commits October 19, 2018 13:51

Fix some tests

00a0624

appRoot is not Nullable

c3b0eed

Migrate Gradle tests for JibConfigurationProcessor

e677850

RawConfigurations --> RawConfiguration

53c8d78

chanseokoh added the PR: Not Ready label Oct 19, 2018

chanseokoh added 3 commits October 19, 2018 16:51

WIP - migrate more Gradle tests

bd93804

WIP - migrate more tests / fix tests

98fe653

Fix things and tests

0c2c341

chanseokoh force-pushed the refactor-PluginConfigurationProcessor branch from 240beca to 0c2c341 Compare October 22, 2018 21:36

chanseokoh added 3 commits October 23, 2018 11:38

Remove PluginConfigurationProcessors

b3d6d0e

Rename to PluginConfigurationProcessor

b833025

Format code

43e8ff8

chanseokoh mentioned this pull request Oct 23, 2018

Consider removing ProjectProperties.isWarProject() #1088

Closed

chanseokoh added 2 commits October 23, 2018 12:12

Refactor more code

872a142

Add Javadocs

cb259c0

chanseokoh removed the PR: Not Ready label Oct 23, 2018

Merge branch 'master' into refactor-PluginConfigurationProcessor

5324187

coollog reviewed Oct 24, 2018

View reviewed changes

chanseokoh added 4 commits October 25, 2018 13:11

package-private and private constructors

6af8a75

Remove unnecessary fields

cd58046

Migrate Nullable to Optional

acc2eee

Merge remote-tracking branch 'origin/master' into refactor-PluginConf…

c143a8d

…igurationProcessor

coollog reviewed Oct 26, 2018

View reviewed changes

Rename exception and variable

137b3d5

coollog reviewed Oct 26, 2018

View reviewed changes

chanseokoh added 3 commits October 26, 2018 12:43

review comments

a61779a

Clean up

ac8d8ab

Clean up

f8a233b

TadCordle reviewed Oct 29, 2018

View reviewed changes

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

jib-maven-plugin/src/test/java/com/google/cloud/tools/jib/maven/MavenRawConfigurationTest.java Show resolved Hide resolved

TadCordle approved these changes Oct 29, 2018

View reviewed changes

chanseokoh added 2 commits October 29, 2018 16:59

Merge branch 'master' into refactor-PluginConfigurationProcessor

6ad83de

Merge remote-tracking branch 'origin/master' into refactor-PluginConf…

16eda86

…igurationProcessor

chanseokoh merged commit af00e2d into master Oct 29, 2018

chanseokoh deleted the refactor-PluginConfigurationProcessor branch October 29, 2018 21:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Share PluginConfigurationProcessor between Gradle and Maven #1163

Share PluginConfigurationProcessor between Gradle and Maven #1163

chanseokoh commented Oct 18, 2018 •

edited

Loading

coollog commented Oct 19, 2018

chanseokoh commented Oct 19, 2018

coollog commented Oct 19, 2018

chanseokoh commented Oct 19, 2018

chanseokoh commented Oct 19, 2018

coollog commented Oct 19, 2018

chanseokoh commented Oct 19, 2018

chanseokoh commented Oct 23, 2018

chanseokoh commented Oct 23, 2018 •

edited

Loading

TadCordle commented Oct 23, 2018

chanseokoh commented Oct 23, 2018 •

edited

Loading

chanseokoh commented Oct 23, 2018

chanseokoh commented Oct 23, 2018

coollog Oct 23, 2018

chanseokoh Oct 25, 2018

coollog Oct 23, 2018

chanseokoh Oct 25, 2018

coollog Oct 26, 2018

chanseokoh Oct 26, 2018

TadCordle commented Oct 24, 2018

coollog Oct 26, 2018

coollog Oct 26, 2018

coollog left a comment

coollog Oct 26, 2018

chanseokoh Oct 26, 2018

Share PluginConfigurationProcessor between Gradle and Maven #1163

Share PluginConfigurationProcessor between Gradle and Maven #1163

Conversation

chanseokoh commented Oct 18, 2018 • edited Loading

coollog commented Oct 19, 2018

chanseokoh commented Oct 19, 2018

coollog commented Oct 19, 2018

chanseokoh commented Oct 19, 2018

chanseokoh commented Oct 19, 2018

coollog commented Oct 19, 2018

chanseokoh commented Oct 19, 2018

chanseokoh commented Oct 23, 2018

chanseokoh commented Oct 23, 2018 • edited Loading

TadCordle commented Oct 23, 2018

chanseokoh commented Oct 23, 2018 • edited Loading

chanseokoh commented Oct 23, 2018

chanseokoh commented Oct 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TadCordle commented Oct 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coollog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chanseokoh commented Oct 18, 2018 •

edited

Loading

chanseokoh commented Oct 23, 2018 •

edited

Loading

chanseokoh commented Oct 23, 2018 •

edited

Loading