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.

Sign up for GitHub

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

Jump to bottom

Use JavaContainerBuilder in plugins #1572

Merged
merged 22 commits into from
Mar 27, 2019
Merged

Use JavaContainerBuilder in plugins #1572

merged 22 commits into from
Mar 27, 2019

Conversation

TadCordle
Copy link
Contributor

@TadCordle TadCordle commented Mar 21, 2019 •
edited
Loading

Fixes #1373.

Notable changes:

  • Replaced all uses of JavaLayerConfigurations in plugins with JavaContainerBuilder (aside from a couple global constants; should these be moved?)
  • Added ProjectProperties.getClassFiles(); since project properties no longer hold layer configurations, we need another way to pass a list of class files to MainClassResolver
  • GradleLayerConfigurations and MavenLayerConfigurations were consolidated enough that I decided to just move their logic straight into GradleProjectProperties/MavenProjectProperties
  • As such, GradleLayerConfigurationsTest and MavenLayerConfigurationsTest were moved into their respective ProjectProperties tests
  • The extra files layer is added in PluginConfigurationProcessor instead of the ProjectProperties, to avoid passing around too many parameters/share code between the plugins
  • Some redundant info-level log messages were removed; they showed which files were added to the container, but most of the same information is logged on containerize()

chanseokoh
chanseokoh reviewed Mar 21, 2019
View reviewed changes
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.

Another early set of comments. (Sorry, there may be another set later, due to the magnitude of the API/structural changes.)

jib-core/src/main/java/com/google/cloud/tools/jib/api/JavaContainerBuilder.java Outdated Show resolved Hide resolved
jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/GradleProjectProperties.java Outdated Show resolved Hide resolved
jib-core/src/main/java/com/google/cloud/tools/jib/api/JavaContainerBuilder.java Outdated Show resolved Hide resolved
jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/GradleProjectProperties.java Outdated Show resolved Hide resolved
@TadCordle TadCordle marked this pull request as ready for review March 22, 2019 19:03
@TadCordle TadCordle requested a review from a team March 25, 2019 15:31
chanseokoh
chanseokoh reviewed Mar 25, 2019
View reviewed changes
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.

Moving the extra layer code out of ProjectProperties looks nice. I'll go over the PR once more. Sorry for the delay.

...on/src/main/java/com/google/cloud/tools/jib/plugins/common/PluginConfigurationProcessor.java Show resolved Hide resolved
briandealwis
briandealwis reviewed Mar 25, 2019
View reviewed changes
Copy link
Member

@briandealwis briandealwis left a comment

Choose a reason for hiding this comment

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

Here are some comments. I need to make another pass through this; so many helpers.

jib-core/src/main/java/com/google/cloud/tools/jib/api/JavaContainerBuilder.java Outdated Show resolved Hide resolved
jib-core/src/main/java/com/google/cloud/tools/jib/api/JavaContainerBuilder.java Outdated Show resolved Hide resolved
jib-core/src/main/java/com/google/cloud/tools/jib/api/JavaContainerBuilder.java Outdated Show resolved Hide resolved
...mmon/src/main/java/com/google/cloud/tools/jib/plugins/common/JavaContainerBuilderHelper.java Outdated Show resolved Hide resolved
jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/GradleProjectProperties.java Show resolved Hide resolved
.../src/test/java/com/google/cloud/tools/jib/plugins/common/JavaContainerBuilderHelperTest.java Outdated Show resolved Hide resolved
@chanseokoh
Copy link
Member

LGTM for what I can check. I'll leave @briandealwis to approve this.

briandealwis
briandealwis approved these changes Mar 26, 2019
View reviewed changes
Copy link
Member

@briandealwis briandealwis left a comment

Choose a reason for hiding this comment

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

Could we have a better name than getContainerBuilderWithLayers? It's really starting the containerization process. The {Gradle,Maven}ProjectProperties doesn't seem like the right place for this, or maybe they just need to be renamed. That can be done later.

jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/MavenProjectProperties.java
public JibContainerBuilder getContainerBuilderWithLayers(RegistryImage baseImage)
throws IOException {
try {
if (isWarProject()) {
Copy link
Member

Choose a reason for hiding this comment

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

Ditto my comment in GradleProjectProperties: it feels like we should have MavenWarProjectProperties and MavenJarProjectProperties.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'll file an issue for this.

jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/GradleProjectProperties.java
public JibContainerBuilder getContainerBuilderWithLayers(RegistryImage baseImage) {
try {
if (isWarProject()) {
logger.info("WAR project identified, creating WAR image: " + project.getDisplayName());
Copy link
Member

Choose a reason for hiding this comment

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

So this seems… out of place. It feels like we should have subclasses a GradleWarProjectProperties and GradleJarProjectProperties.

@TadCordle TadCordle merged commit d69aec5 into master Mar 27, 2019
@TadCordle TadCordle deleted the i1373-javacontainerbuilder-plugins branch March 27, 2019 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chanseokoh chanseokoh chanseokoh left review comments

@briandealwis briandealwis briandealwis approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@TadCordle @chanseokoh @briandealwis @googlebot