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

Support for getting / injecting shared Maven components in JibMavenPluginExtension #3036

Closed
stefanocke opened this issue Feb 7, 2021 · 9 comments

Comments

@stefanocke
Copy link
Contributor

stefanocke commented Feb 7, 2021

I would like to achieve the following in an extension plugin (maybe as a pull request for jib-layer-filter-extension-maven):

  • determine the dependencies (including transitive) that come from the parent pom
  • put them into one or more separate layers

The idea is that the parent pom defines the "company / project standard" which rarely changes.
While the existing jib-layer-filter-extension-maven is already quite useful, it would be still quite cumbersome to write globs for all direct and transitive dependencies.
For example, think about having some Spring Boot starters in the parent pom for all your microservice, but some other Spring Boot starters are optional and only used by specific microservices. You not only would need quite exact globs for the Spring Boot starters then, but also for their manyfold transitive dependencies.

So far the introduction...

Now my proposal / request:

For doing advanced tasks like "determine the dependencies (including transitive) that come from the parent pom" in an extension, I need shared Maven components like ProjectDependenciesResolver.

In a Mojo, one would get it like this

    @Component
    ProjectDependenciesResolver resolver;

But the JibMavenPluginExtension is no Mojo, so I can't do that.

So far, the following works, but the lookup() method is already deprecated and will be removed in Maven 4:

      ProjectDependenciesResolver resolver = (ProjectDependenciesResolver) mavenData.getMavenSession()
          .lookup(org.apache.maven.project.ProjectDependenciesResolver.class.getName());

So do you see any chance to perform dependency injection on the loaded extensions or to provide some other (compatible) way to get shared Maven components?

You can see a first working draft of my approach here stefanocke/jib-extensions@d9652b5

The deprecated lookup() method is used here stefanocke/jib-extensions@d9652b5#diff-1cf0b87bb4b940a4278b65d96c8da263fc6457a431798f0b1fb7fd2f648c17e6R200

@chanseokoh
Copy link
Member

chanseokoh commented Feb 11, 2021

Hi @stefanocke,

I think this is a very interesting idea.

It's just that we are unsure what would be the best architecture to enable this with enough flexibility and expandability. Should Jib just pass ProjectDependenciesResolver as another argument in tandem with MavenProject? That's the simplest solution, but what if the users want other Maven components as well later? It doesn't feel right to just keep adding components. It'd be nice if the extension framework could somehow inject components (as Maven does with @Component as you said), but I don't know if that's technically possible? (We don't have much expertise with implementing dependency injection with annotations.)

What's your thoughts? Any detailed design proposal?

BTW, I think we have to leave this as a low priority. Not that we think this is not important, but our resources are very limited at the moment. We also welcome contributions.

@stefanocke
Copy link
Contributor Author

stefanocke commented Feb 11, 2021

Thanks for your feedback, @chanseokoh. I agree with you that just passing in ProjectDependenciesResolver is not a good solution.
Regarding Depedency injection in Maven I only know that it was "Plexus container" in former times and it moved to Guice. See https://maven.apache.org/maven-jsr330.html

However, I don't know enough yet, whether there is any way to apply this DI to our own (non-Mojo) components.

Some hint from someone who is deeper into Maven internals would be helpful. But of course, the solution should be based on some public API of Maven.

If I find out some more details by myself, I will come back here...

@stefanocke
Copy link
Contributor Author

Okay, so approximately my plan would be to make the extension a Sisu / Guice / JSR-330 component and to inject it into the jib plugin instead of creating it by using the service loader.

Pretty much as the Jsr330Component in the example on https://maven.apache.org/maven-jsr330.html

I am not sure yet, if jib has completely move from plexus to Sisu / Guice / JSR-330 then or if it is possible to mix.
Even a complete migration should not be a to big deal, since as I understand it it mainly about replacing the two or three existing @Component annotations in the Mojo by @Inject.

Sisu / Guice / JSR-330 needs at least Maven 3.1.0. So, jib would not work for older Maven versions. Would this be an issue, @chanseokoh?

@chanseokoh
Copy link
Member

Thanks for looking into it! I think I am not following all the technical stuff you said (😅 hopefully it's easy to implement and doesn't incur a lot of changes). However, At least I can say requiring 3.1.0+ is OK. I see 3.1.0 was released in 2015.

stefanocke added a commit to stefanocke/jib that referenced this issue Feb 14, 2021
injection of extensions
@stefanocke
Copy link
Contributor Author

stefanocke commented Feb 14, 2021

@chanseokoh , I have implemented the approach in the commit stefanocke@0c122a1.
It is IMHO completely compatible and optional and quite lean, too. If the extension is not injected, the plugin still uses the existing JDK service loader approach to create it.

Basically the solution comes down to inject the extensions into the MOJO like this:

public abstract class JibPluginConfiguration extends AbstractMojo {
  ...
  @Inject
  private Set<JibMavenPluginExtension<?>> injectedPluginExtensions = Collections.emptySet();

The extension just needs to declare itsfelf as a JSR-330 / Sisu Component by adding the following annotations:

@Named
@Singleton
public class JibLayerFilterExtension implements JibMavenPluginExtension<Configuration> {
...

And finally, some index is required in META-INF/sisu/javax.inject.Named of the extension:

com.google.cloud.tools.jib.maven.extension.layerfilter.JibLayerFilterExtension

Under the hoods, Sisu will discover the so declared component and apply dependency injection to it, which allows me to accomplish my original goal to inject shared Maven components into the extension like this:

@Named
@Singleton
public class JibLayerFilterExtension implements JibMavenPluginExtension<Configuration> {
  ...
  @Inject
  private ProjectDependenciesResolver dependencyResolver;

Could you please have a look? If this approach is okay with you, I would add some integration test and create a pull request.

@chanseokoh
Copy link
Member

Thanks a lot for doing this! We'll get back to this as soon as we have time, but please be patient until then.

stefanocke added a commit to stefanocke/jib-extensions that referenced this issue Feb 20, 2021
injection of extensions

shows how to use dependency injection in an extension
stefanocke added a commit to stefanocke/jib that referenced this issue Feb 20, 2021
injection of extensions - test
@stefanocke
Copy link
Contributor Author

stefanocke commented Feb 20, 2021

It needed some polishing, but now everything should be ready in the pull request.

@chanseokoh chanseokoh added this to the v2.9.0 milestone Feb 24, 2021
chanseokoh pushed a commit to GoogleContainerTools/jib-extensions that referenced this issue Mar 4, 2021
…or dependencies from parent POM (#76)

* Moving parent dependencies to separate layers - first draft

* Maps of dependencies instead of Sets

to support better error handling

* GoogleContainerTools/jib#3036 support
injection of extensions

shows how to use dependency injection in an extension

* configuration for createParentLayers

And formatting and logging.

* some tests

* some more tests

* googleJavaFormat

* Review, first part

* Review: fixed all remaining minor issues. Left the more interesting ones
still open for further consideration.

* getBaseVersion instead of getVersion

* match filename instead of complete extraction path

to not depend on "app/libs", which is connfigurable in jib

* log potential matches when parent dependency not found

use source file name for matching instead of filename from extraction
path

* Use complete source path instead of filename for matching

Especially on the Artifact side that means that the filename is not
"constructed" anymore by the extension. We make no assumption about the
structure of the source file name or path.
The only assumption that is left is, that Maven will resolve to the same
file path, if the same artifact coordinates are resolved multiple times
within a build / session.

* Review: some minor issues
chanseokoh pushed a commit that referenced this issue Mar 4, 2021
…uginExtension (#3062)

* #3036 support
injection of extensions

* #3036 support
injection of extensions - test

* fix gradle plugin validation error

* fix checkstyly

* fix checkstyle

* fix checkstyle

* fix googleJavaFormat

* fix googleJavaFormat

* fix checkstyle again :-(

* increase test coverage

* polishing: avoid type casting for extension

* reworked: injected extensions should not be part of extension
configuration

... but just another extensionLoader mechanism

* test coverage

* refactoring: pull up getMavenProjectProperties

and test it

* Revert "refactoring: pull up getMavenProjectProperties"

This reverts commit 279418c.

* Review
@chanseokoh
Copy link
Member

Fixed by #3062. This new approach to load Jib extensions (which enables injecting Maven components) will be documented in the jib-extensions README.

@chanseokoh
Copy link
Member

@stefanocke thanks for your great contribution! This was really a cool idea, and we were very happy to get help from an expert on this subject.

We released Jib 3.0, and you should be able to start using the feature. (I know I am lazy to update the Jib extensions doc.)

chanseokoh added a commit to GoogleContainerTools/jib-extensions that referenced this issue Apr 9, 2021
…eating parent dependency layers in jib-layer-filter-extension-maven (#81)

* Documentation for
GoogleContainerTools/jib#3036 and
#76

* Update README.md

Co-authored-by: Chanseok Oh <chanseok@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants