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

fix(core): Externally built Integrations run command configuration from jvm trait #5151

Merged
merged 1 commit into from
Feb 13, 2024

Conversation

gansheer
Copy link
Contributor

@gansheer gansheer commented Feb 9, 2024

Closes #5112

  • change the check to evaluate if the kit is external or not instead of 'camel-k-kit' naming convention
  • let the possibility for the user to explicitly enable the trait jvm and add a condition to inform the user that it is disabled by default
  • enable jvm trait explicitly on promote command

Release Note

fix(core): Externally built Integrations run command configuration from jvm trait

Copy link
Contributor

github-actions bot commented Feb 9, 2024

✔️ Unit test coverage report - coverage increased from 35.6% to 35.7% (+0.1%)

@gansheer gansheer force-pushed the fix/5112_external_build_integration branch from 1b7ac66 to 5cd3425 Compare February 9, 2024 16:29
Copy link
Contributor

✔️ Unit test coverage report - coverage increased from 35.6% to 35.7% (+0.1%)

@gansheer
Copy link
Contributor Author

Could someone trigger the failing test again please 🙏

@gansheer gansheer force-pushed the fix/5112_external_build_integration branch from 5cd3425 to 053f387 Compare February 12, 2024 13:27
Copy link
Contributor

✔️ Unit test coverage report - coverage increased from 35.6% to 35.7% (+0.1%)

* change the check to evaluate if the kit is external or not instead of 'camel-k-kit' naming convention
* let the possibility for the user to explicitly enable the trait jvm and add a condition to inform the user that it is disabled by default
* enable jvm trait explicitly on promote command
@gansheer gansheer force-pushed the fix/5112_external_build_integration branch from 053f387 to e65f156 Compare February 13, 2024 10:03
Copy link
Contributor

✔️ Unit test coverage report - coverage increased from 35.7% to 35.8% (+0.1%)

@gansheer
Copy link
Contributor Author

I think the PR is good except for known flacky tests.
Additionnaly, I explicitly set the jvm as enabled in promote command when not defined.

@@ -468,6 +469,12 @@ func (o *promoteCmdOptions) editIntegration(it *v1.Integration) *v1.Integration
dst.Spec.Traits.Container = &traitv1.ContainerTrait{}
}
dst.Spec.Traits.Container.Image = contImage
if dst.Spec.Traits.JVM == 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 don't think we need this check. We know the promote tests are kind of flaky, no need to add anything in the command IMO. The container has to be initialized because it could raise a nil exception, but not the JVM trait, for which, we must keep the default setting.

Copy link
Contributor Author

@gansheer gansheer Feb 13, 2024

Choose a reason for hiding this comment

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

It is not a flacky test in this case, the promote command run the "promoted" integration with the trait Container.Image, so it becomes an external kit and does not start correctly. Since the trait was not explicilty set in the original integration, we are relying on default behavior, which will be different for original and promoted integration. This aims to fix this issue.

The alternative would be to add support for the traits on the comand options or add the copy of the integration kit created for the image.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. We can proceed then.

@@ -468,6 +469,12 @@ func (o *promoteCmdOptions) editIntegration(it *v1.Integration) *v1.Integration
dst.Spec.Traits.Container = &traitv1.ContainerTrait{}
}
dst.Spec.Traits.Container.Image = contImage
if dst.Spec.Traits.JVM == 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 see. We can proceed then.

@squakez squakez merged commit 9d35897 into apache:main Feb 13, 2024
11 of 15 checks passed
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.

Externally built Integrations are deployed without a command in Camel-K 2.2.0
2 participants