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

Add and enable new plugin extension configuration in Maven #2420

Merged
merged 12 commits into from
Apr 27, 2020

Conversation

chanseokoh
Copy link
Member

@chanseokoh chanseokoh commented Apr 20, 2020

Adds new extension configuration parameters for Maven. Extensions can additionally get custom properties. The order of executing extensions will be the order defined in pom.xml.

<extensions>
  <extension>
    <implementation>com.google.cloud.tools.jib.plugins.extension.maven.quarkus.QuarkusJibExtension</implementation>
    <properties>
      <customProperty>value</customProperty>
    </properties>
  </extension>
  <extension>
    <implementation>com.example.FooExtension</implementation>
  </extension>
  <!-- Not yet supported
  <extension>com.example.BarExtension</extension>
  -->
</extensions>

@chanseokoh chanseokoh changed the title DRAFT: add and enable new plugin extension configuration in Maven Add and enable new plugin extension configuration in Maven Apr 22, 2020
@chanseokoh chanseokoh marked this pull request as ready for review April 22, 2020 16:28
@@ -30,6 +30,13 @@
*/
public interface RawConfiguration {
Copy link
Member

Choose a reason for hiding this comment

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

Just curious? Do we have to modify raw configuration if we're already calling back into ProjectProperties?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question. It's a viable option that I've considered too. There are pros and cons. ProjectProperties doesn't have access to configuration values. Therefore, we need to change the constructor of MavenProjectProperties and pass extension config values from each MOJO (such as BuildImageMojo) where it instantiate ProjectProperties. Then we can think of a few ways to pass the information:

  • Pass JibPluginConfiguration. But this seems weird considering that there's already a higher abstraction of RawConfiguration. I don't like the divergence and incoherence in passing around config values. Also too much information and power given to ProjectProperties.
  • Pass MavenRawConfiguration. Requires modifying it anyway. Better than passing JibPluginConfiguration, but it still feels like passing RawConfiguration in the constructor unnecessarily provides too much information. It only needs the extension config.
  • Each MOJO passes only ExecutionConfiguration. Seems better than previous options, but I thought perhaps it's better to maintain consistency in obtaining config values from the common interface (RawConfiguration) that serves as the source of truth.
  • Considerable refactoring as to who runs extensions: don't run extensions in ProjectProperties. runPluginExtensions don't use any instance values of ProjectProperties, so in fact, each MOJO can directly run extensions. The only reason its in ProjectProperties is enable the common PluginConfigurationProcess to call Maven- and Gradle-specific extension execution routine. If you remember, I briefly mentioned this kind of refactoring a while ago, as it was related to whether I should have a common API base project or not; I eventually decided to have PluginConfigurationProcess call the routine instead of each MOJO runs executions, as one of the downsides was a considerable amount of refactoring.

Copy link
Member

@loosebazooka loosebazooka left a comment

Choose a reason for hiding this comment

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

I think this looks okay. Just curious about some types and stuff

@chanseokoh chanseokoh merged commit 9b35a0a into master Apr 27, 2020
@chanseokoh chanseokoh deleted the plugin-extension-java-src-new-config branch April 27, 2020 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants