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

Remove jib from apps-generator plugin #548

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

corneil
Copy link
Collaborator

@corneil corneil commented Jun 4, 2024

No description provided.

@corneil corneil requested a review from onobc June 4, 2024 12:24
<image>
<name>{{app.containerImage.orgName}}/${project.artifactId}:{{app.containerImage.tag}}</name>
</image>
{{/app.containerImage.enableMetadata}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to specify any of the options like before? Or does the boot image already set these?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added tests

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Boot Build Image requires all labels in one environment variable. The JSON breaks the parsing of this.

<plugin>
<groupId>org.codehaus.mojo</groupId>
<artifactId>properties-maven-plugin</artifactId>
<version>1.0.0</version>
<version>1.2.1</version>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please put each of the version updates is a separate commit. In this PR is fine, but lets keep them separate for history sake (and be sure we do not squash - but rather rebase and merge).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@@ -99,15 +99,8 @@ public void testSomething() throws Exception {

assertThat(plugins.stream().filter(p -> p.getArtifactId().equals("spring-boot-maven-plugin")).count()).isEqualTo(1);
assertThat(plugins.stream().filter(p -> p.getArtifactId().equals("properties-maven-plugin")).count()).isEqualTo(1);
assertThat(plugins.stream().filter(p -> p.getArtifactId().equals("jib-maven-plugin")).count()).isEqualTo(1);

Plugin jibPlugin = plugins.stream().filter(p -> p.getArtifactId().equals("jib-maven-plugin")).findFirst().get();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any replacements we need to make here to add coverage for the spring boot generated image?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added

@@ -128,13 +127,6 @@ public void testWithDisabledContainerMetadata() throws Exception {
// The properties-maven-plugin should not be defined if the container metadata is not enabled.
assertThat(plugins.stream().filter(p -> p.getArtifactId().equals("properties-maven-plugin")).count()).isEqualTo(0);

assertThat(plugins.stream().filter(p -> p.getArtifactId().equals("jib-maven-plugin")).count()).isEqualTo(1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

Copy link
Collaborator

@onobc onobc left a comment

Choose a reason for hiding this comment

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

All in all looks good @corneil .

I have a few minor changes requested.

We should also consider moving the version number up to either 1.1.0 or 2.0.0 to signal the breaking change of the jib removal.

@corneil corneil force-pushed the corneil/remove-jib-from-plugin branch from 80e57eb to 8f1cf4e Compare June 5, 2024 12:19
Provide for spring-boot-maven-plugin configuration for build-image.
Provide tests for spring-boot-maven-plugin configuration.
@corneil corneil force-pushed the corneil/remove-jib-from-plugin branch from 2279b38 to 7fb4765 Compare June 5, 2024 12:45
@corneil
Copy link
Collaborator Author

corneil commented Jun 5, 2024

Note: The apps-plugin-pr workflow build successfully. Even though ci-pr was triggered as well because there are file changes outside spring-cloud-dataflow-apps-plugin

@corneil corneil requested a review from onobc June 5, 2024 12:50
Add nativePlugin profile.
spring-boot:build-image -Pnative should suffice.
@corneil corneil force-pushed the corneil/remove-jib-from-plugin branch from 9410a09 to cb6c08c Compare July 31, 2024 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants