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

feat(#1656) : Add Jib publish strategy #4680

Merged
merged 2 commits into from
Aug 30, 2023

Conversation

gansheer
Copy link
Contributor

@gansheer gansheer commented Aug 21, 2023

Addition of a new strategy leveraging Jib:

  • Add publish jib strategy compatible with incremental build and native build
  • Use google jib maven plugin
  • Use builder maven profiles trait to configure jib plugin
    • The ConfigMap is created with:
      • maven jib plugin profile content in profile.xml
      • kit as an owner
    • The Jib Profile is used in the Jib published strategy

What will be done in later PRs:

  • extract plugin version in camel-k-runtime
  • refactor the way we interact with maven to be able to reuse the code to generate a complete maven command in the publish task instead of using a temp file "MAVEN_CONTEXT"

Release Note

feat(#1656) : Add Jib publish strategy

@github-actions
Copy link
Contributor

🐫 Thank you for contributing!

Code Coverage Report ⚠️ - Coverage changed: 39.2% --> 39.0% (Coverage difference: -.2%)

@gansheer gansheer force-pushed the feature/1656_jib_plugin_strategy branch from ded8201 to c1f28f7 Compare August 21, 2023 16:37
@github-actions
Copy link
Contributor

🐫 Thank you for contributing!

Code Coverage Report ⚠️ - Coverage changed: 39.2% --> 39.0% (Coverage difference: -.2%)

jibPlugin := maven.Plugin{
GroupID: "com.google.cloud.tools",
ArtifactID: "jib-maven-plugin",
Version: "3.3.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

We should have this variable set in the Camel K Runtime BOM instead but I guess can be considered after the draft phase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, I would need to see how to make that work. Is it a good idea to do that now as we are moving the runtime code to a possible camel-quarkus extension ?

Copy link
Contributor

Choose a reason for hiding this comment

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

It does not matter, we can always add that, let's keep for a later dev.

pkg/builder/quarkus.go Outdated Show resolved Hide resolved
pkg/util/jib/configuration.go Show resolved Hide resolved
@gansheer gansheer force-pushed the feature/1656_jib_plugin_strategy branch from c1f28f7 to f447c9d Compare August 22, 2023 06:41
@github-actions
Copy link
Contributor

🐫 Thank you for contributing!

Code Coverage Report ⚠️ - Coverage changed: 39.2% --> 39.0% (Coverage difference: -.2%)

@gansheer gansheer force-pushed the feature/1656_jib_plugin_strategy branch from f447c9d to 0b149a1 Compare August 22, 2023 06:52
@github-actions
Copy link
Contributor

🐫 Thank you for contributing!

Code Coverage Report ⚠️ - Coverage changed: 39.2% --> 39.0% (Coverage difference: -.2%)

@gansheer gansheer force-pushed the feature/1656_jib_plugin_strategy branch from 0b149a1 to 3e16b4d Compare August 22, 2023 15:53
@github-actions
Copy link
Contributor

🐫 Thank you for contributing!

Code Coverage Report ⚠️ - Coverage changed: 39.0% --> 38.8% (Coverage difference: -.2%)

Copy link
Contributor

@squakez squakez left a comment

Choose a reason for hiding this comment

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

In general I think it's okey. There are a few design changes that could make this cleaner if possible.

@@ -206,6 +207,7 @@ func GenerateQuarkusProjectCommon(runtimeVersion string, quarkusVersion string,
},
},
},
jib.JibMavenPlugin(),
Copy link
Contributor

Choose a reason for hiding this comment

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

We should do this one only when the publish strategy is JIB. However, in general, I think we should leverage the builder profile configuration to have a clean logic separation (ie, the quarkus trait should know nothing about the publishing strategy).

Copy link
Contributor

Choose a reason for hiding this comment

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

With the proposed approach, you should be able to create a configmap on the fly containing the required configuration for Jib and set to the build accordingly.

pkg/builder/jib.go Outdated Show resolved Hide resolved
@@ -142,7 +142,12 @@ func (c *Command) Do(ctx context.Context) error {

Log.WithValues("MAVEN_OPTS", mavenOptions).Infof("executing: %s", strings.Join(cmd.Args, " "))

return util.RunAndLog(ctx, cmd, mavenLogHandler, mavenLogHandler)
// generate maven file
if err := generateMavenContext(c.context.Path, args); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering if, instead of copying the execution config in a file, you could try to regenerate the same at the moment of calling the JIB (ie, creating a func which take care to generate and call it here and later when needed). It would be much cleaner IMO.

Copy link
Contributor Author

@gansheer gansheer Aug 23, 2023

Choose a reason for hiding this comment

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

It would be better, but I would like to dedicate another issue to this part as it needs a big refactoring in a sensitive part of the build task. It would also be an occasion to see if we can separate the maven code dedicated to commands execution from the project generation code.

@gansheer gansheer force-pushed the feature/1656_jib_plugin_strategy branch from 3e16b4d to 8cd46a1 Compare August 23, 2023 14:58
@github-actions
Copy link
Contributor

🐫 Thank you for contributing!

Code Coverage Report ⚠️ - Coverage changed: 39.0% --> 38.8% (Coverage difference: -.2%)

@gansheer gansheer force-pushed the feature/1656_jib_plugin_strategy branch from 229dedf to 63cd5d3 Compare August 24, 2023 15:07
@github-actions
Copy link
Contributor

🐫 Thank you for contributing!

Code Coverage Report ⚠️ - Coverage changed: 39.0% --> 38.9% (Coverage difference: -.1%)

1 similar comment
@github-actions
Copy link
Contributor

🐫 Thank you for contributing!

Code Coverage Report ⚠️ - Coverage changed: 39.0% --> 38.9% (Coverage difference: -.1%)

@gansheer gansheer force-pushed the feature/1656_jib_plugin_strategy branch from 63cd5d3 to 5e89ebe Compare August 24, 2023 15:15
@gansheer
Copy link
Contributor Author

gansheer commented Aug 24, 2023

I adapted to use a maven profile:

  • change the builder profile trait to accept multiple values
  • in case of JIB publish strategy, a configmap containing the default jib profile is created linked to the integration kit

What still needs to be done in this Draft: tests and commits squash

What will be done in later PRs:

  • extract plugin version in camel-k-runtime
  • refactor the way we interact with maven to be able to reuse the code to generate a complete maven command in the publish task instead of using a temp file "MAVEN_CONTEXT"

@squakez

@gansheer gansheer requested a review from squakez August 24, 2023 15:21
@github-actions
Copy link
Contributor

🐫 Thank you for contributing!

Code Coverage Report ⚠️ - Coverage changed: 39.0% --> 38.9% (Coverage difference: -.1%)

pkg/builder/project.go Outdated Show resolved Hide resolved
@gansheer gansheer force-pushed the feature/1656_jib_plugin_strategy branch from 5e89ebe to 7588ab2 Compare August 25, 2023 07:23
@github-actions
Copy link
Contributor

🐫 Thank you for contributing!

Code Coverage Report ⚠️ - Coverage changed: 39.0% --> 38.9% (Coverage difference: -.1%)

@gansheer gansheer force-pushed the feature/1656_jib_plugin_strategy branch from 7588ab2 to 7035a90 Compare August 25, 2023 14:39
@github-actions
Copy link
Contributor

🐫 Thank you for contributing!

Code Coverage Report ⚠️ - Coverage changed: 39.0% --> 38.9% (Coverage difference: -.1%)

* Add publish jib strategy compatible with incremental build and native build
* Use google jib maven plugin
* Use builder maven profiles trait to configure jib plugin
 * The ConfigMap is created with:
   * maven jib plugin profile content in profile.xml
   * kit as an owner
 * The Jib Profile is used in the Jib published strategy
@gansheer gansheer force-pushed the feature/1656_jib_plugin_strategy branch from 7035a90 to c921652 Compare August 25, 2023 14:55
@gansheer gansheer marked this pull request as ready for review August 25, 2023 14:58
@github-actions
Copy link
Contributor

🐫 Thank you for contributing!

Code Coverage Report ⚠️ - Coverage changed: 39.0% --> 38.9% (Coverage difference: -.1%)

Copy link
Contributor

@squakez squakez left a comment

Choose a reason for hiding this comment

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

Excellent work, thanks for contributing to this. I'm going to add some issues to take care of known enhancements later on.

@squakez squakez merged commit 0429052 into apache:main Aug 30, 2023
This was referenced Aug 30, 2023
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.

3 participants