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 #3062

Merged
merged 16 commits into from
Mar 4, 2021

Conversation

stefanocke
Copy link
Contributor

Pull request for #3036.

injection of extensions
injection of extensions - test
@google-cla

This comment has been minimized.

@google-cla google-cla bot added the cla: no label Feb 20, 2021
@stefanocke
Copy link
Contributor Author

@googlebot I signed it!

@google-cla google-cla bot added cla: yes and removed cla: no labels Feb 20, 2021
@codecov
Copy link

codecov bot commented Feb 20, 2021

Codecov Report

Merging #3062 (481fd92) into master (73a6395) will increase coverage by 0.03%.
The diff coverage is 55.55%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3062      +/-   ##
============================================
+ Coverage     71.09%   71.12%   +0.03%     
- Complexity     2307     2315       +8     
============================================
  Files           278      278              
  Lines          9755     9790      +35     
  Branches        989      991       +2     
============================================
+ Hits           6935     6963      +28     
- Misses         2480     2482       +2     
- Partials        340      345       +5     
Impacted Files Coverage Δ Complexity Δ
.../google/cloud/tools/jib/maven/BuildDockerMojo.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...m/google/cloud/tools/jib/maven/BuildImageMojo.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...com/google/cloud/tools/jib/maven/BuildTarMojo.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...le/cloud/tools/jib/maven/skaffold/SyncMapMojo.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
.../cloud/tools/jib/maven/JibPluginConfiguration.java 67.88% <100.00%> (+0.26%) 54.00 <1.00> (+1.00)
.../cloud/tools/jib/maven/MavenProjectProperties.java 82.02% <100.00%> (+0.13%) 73.00 <0.00> (ø)
...om/google/cloud/tools/jib/gradle/JibExtension.java 82.45% <0.00%> (-1.55%) 19.00% <0.00%> (+1.00%) ⬇️
...loud/tools/jib/gradle/GradleProjectProperties.java 65.21% <0.00%> (-0.65%) 31.00% <0.00%> (-2.00%)
...om/google/cloud/tools/jib/gradle/BuildTarTask.java 4.47% <0.00%> (-0.29%) 2.00% <0.00%> (ø%)
.../google/cloud/tools/jib/gradle/BuildImageTask.java 5.00% <0.00%> (-0.27%) 2.00% <0.00%> (ø%)
... and 11 more

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 73a6395...481fd92. Read the comment docs.

Copy link
Member

@chanseokoh chanseokoh left a comment

Choose a reason for hiding this comment

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

This "Maven & JSR-330" stuff (Sisu?) looks like magic.

@chanseokoh
Copy link
Member

@stefanocke question: I see the change is to inject Jib extensions, not that it's to inject a Maven component in an Jib extension. Just in case, I wonder if Sisu can't just inject Maven components for Jib extensions loaded by the JDK server loader? (If that's indeed the case, we wouldn't need this change?)

@stefanocke
Copy link
Contributor Author

stefanocke commented Mar 3, 2021

@chanseokoh , the short answer is: no.

The longer answer:
The goal was to inject a Maven component in the jib extension, which I did in the other pull request (the MavenDependencyResolver).
But to achieve this, the extension must be instantiated and processed by the Sisu Container. That's why I added some annotations to the extension and some META-INF/sisu.
If we just load the extension by service loader, it won't be the one that has been created and processed by Sisu. I will just be a new one that the Service Loader would create by calling "newInstance()".
Of course we could try to "post-process" that instance. But then we have two problems to solve:

  • Somehow to call Sisu Dependency injection programmatically, which is probably not possible at all, or at least not by Maven public APIs.
  • And we would still have to do this in jib maven plugin. With probably even much more code than now.

Or do you mean the extension could do the dependency injection by itself, for exampe in its constructor?
Then - again - we would need some public API in Maven again to to this. Or simply something like "getSharedComponent()".
But this was exactly the "old" approach... the deprecated methods in Maven session.

So to summarize: I am convinced the approach I have choosen is the right one. It is compliant to the only existing Maven documentation regarding Sisu / DI. And I even believe, there is no other working approach at all.

To explain it from a different angle:

It is not the case that we use Sisu just for our extension.
Basically every Mojo / service / component that Maven creates and uses is a Sisu component. And they are connected with each other by dependency injection.
The only thing we do here is to say: "My plugin does not just exist of one Sisu component (the Mojo), but my extensions are also Sisu components".
Not so much magic, indeed ;-)
And you are already using dependency injection in the mojos anyway:

  • the Mojo parameters are injected
  • the Maven project and session are injected.

@chanseokoh
Copy link
Member

the short answer is: no.

Yeah, I expected this. Didn't really thought Sisu can inject things into an instance created by the service loader. Was wondering just in case. I think this approach looks good, and perhaps this is what we can recommend going forward.

Copy link
Member

@chanseokoh chanseokoh left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!

@chanseokoh
Copy link
Member

Thanks again for this cool feature!

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.

3 participants