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(trait): define synthetic Kit #5399

Merged
merged 1 commit into from
Apr 23, 2024
Merged

Conversation

squakez
Copy link
Contributor

@squakez squakez commented Apr 22, 2024

While working on #5378 I realized the concept of external Kit is used widely also for the concept of what is we can define as a synthetic Kit. For this user generated container image (likely not from the operator build) we cannot make any assumption when it come to certain traits which require the presence of a CamelCatalog bound to a given runtime provider and version.

This PR would define the new type of kit, giving the possibility to the user to leverage container.imageWasKit=true configuration to instruct the operator that the image imported (ie, via kamel run --image) was coming from a Kit, hence, it can make all the assumptions about it. Conversely, in presence of a "synthetic" kit, the operator cannot make any assumption, above all when coming to the execution of JVM trait.

Prerequisite of #5378

Release Note

fix(trait): define synthetic Kit

While working on apache#5378 I realized the concept of external Kit is used widely also for the concept of what is a Synthetic Kit.

An external Kit is a kit coming from a build from the operator, whilst a synthetic Kit is not coming from an operator.
@squakez squakez merged commit a5c2ce7 into apache:main Apr 23, 2024
15 checks passed
@squakez squakez deleted the fix/synthetic_ik branch April 23, 2024 08:20
@lburgazzoli
Copy link
Contributor

lburgazzoli commented Apr 24, 2024

Sorry for being late but wonder if a better name would have been container.image-format = camel-k|unknown|... or similar, as

  • it is possible to manually create a kit with an external image (I think there is also a cli command)
  • it is possible to build a perfectly compatible image offline

Another observation is that, the IntegrationKit was meant to be a description of an image you can use to run an integration so, probably the image-format is more a kit specific trait. The original IntegrationKit UX was that, i.e if you want to move an integration across environments, then you also have to move the related kit, if not, the operator would have to treat the image as an opaque entity which is assumed to honor the camel-k contract.

@squakez
Copy link
Contributor Author

squakez commented Apr 24, 2024

@lburgazzoli that's exactly the goal of this PR, regardless the name used for the parameter. Definitely, image-format sounds better and I'll work to do this change which result more descriptive. With this, we clarify the intent of an external IntegrationKit, which is either created via the operator (hence, reusable with a bound CamelCatalog, making all the assumptions around it) or created by the user with any other mean (hence, not reusable as it misses the CamelCatalog which generated it).

@lburgazzoli
Copy link
Contributor

lburgazzoli commented Apr 24, 2024

I wonder if this should be a kit only property/annotation rather than a trait, in fact even if the container image was built by camel-k, the absence of a kit may not make it 100% usable (my memory may not be 100% up to date, but the kit was also supposed to carry some traits, even some runtime traits so that all the integration executed with that kit had some common characteristics)

@squakez
Copy link
Contributor Author

squakez commented Apr 24, 2024

I think we're talking about slight different things then. The problem we have is that when the user is running an Integration with container.image defined, the operator cannot know if this image was either built previously by the operator or by any external system. This is happening because the user can only input the container image, so, the operator cannot make any assumption around it, unless the user provide a further configuration to tell the operator that this container image is coming from a previous IntegrationKit.

I understand that you're proposing to force the possibility to run a container image previously built by the operator only with the associated IntegrationKit (which must be copied from its source), is it correct? This feature already exists (ie, kamel run --kit) and this PR should not have affected it at all.

In that case what we need to do is just removing the new parameter included in this PR (only the parameter as the rest of the PR goes in the direction to let a user run an opaque container image without the execution of those traits that need an IntegrationKit). And we need to adjust our test, as, the main reason of this PR was to address certain failing tests that were assuming that we can run an Integration with a container.image coming off from a previous external IntegrationKit.

@lburgazzoli
Copy link
Contributor

I understand that you're proposing to force the possibility to run a container image previously built by the operator only with the associated IntegrationKit (which must be copied from its source), is it correct? This feature already exists (ie, kamel run --kit) and this PR should not have affected it at all.

It is a little bit broader than this because as today you can instruct camel-k to run an integration against a pre-existing container image in two ways:

  1. by creating an external IntegrationKit with the given image and reference it in the Integration spec
  2. by setting the image on the container trait (which then generates an external IntegrationKit behind the scenes, then point 1)
  3. by copying the IntegrationKit and reference it in the Integration spec

Now I think is where things gets a little bit confusing as yes you can use kamel run --kit, but such kit may or may not be built using a camel-k compatible image so you could potentially have to set the image format i.e. with kamel run --kit ... --container.image-format=..., even if you are referencing a kit which, by definition, is expected to describe an image.

This is why I think, the information about the image format is more a Kit specific trait (maybe it can be part of the spec, along with the image property).

the main reason of this PR was to address certain failing tests that were assuming that we can run an Integration with a container.image coming off from a previous external IntegrationKit.

I think @valdar did some work in #5119 to fix this issue , may be worth having a look.

squakez added a commit to squakez/camel-k that referenced this pull request Apr 24, 2024
As discussed in apache#5399 we need to clarify the intent of each IntegrationKit types.

* removal of imageWasKit
* usage of external IntegrationKit when we are certain that the image comes from an IK
* usage of syntetic IntegrationKit when the image are coming from any container image
* rework of promote command to clone an external IntegrationKit beside the Integration and use it accordingly.

Closes apache#5408
squakez added a commit that referenced this pull request Apr 25, 2024
As discussed in #5399 we need to clarify the intent of each IntegrationKit types.

* removal of imageWasKit
* usage of external IntegrationKit when we are certain that the image comes from an IK
* usage of syntetic IntegrationKit when the image are coming from any container image
* rework of promote command to clone an external IntegrationKit beside the Integration and use it accordingly.

Closes #5408
squakez added a commit to squakez/camel-k that referenced this pull request Apr 25, 2024
As discussed in apache#5399 we need to clarify the intent of each IntegrationKit types.

* removal of imageWasKit
* usage of external IntegrationKit when we are certain that the image comes from an IK
* usage of syntetic IntegrationKit when the image are coming from any container image
* rework of promote command to clone an external IntegrationKit beside the Integration and use it accordingly.

Closes apache#5408
squakez added a commit that referenced this pull request Apr 25, 2024
As discussed in #5399 we need to clarify the intent of each IntegrationKit types.

* removal of imageWasKit
* usage of external IntegrationKit when we are certain that the image comes from an IK
* usage of syntetic IntegrationKit when the image are coming from any container image
* rework of promote command to clone an external IntegrationKit beside the Integration and use it accordingly.

Closes #5408
squakez added a commit to jboss-fuse/camel-k that referenced this pull request Apr 25, 2024
As discussed in apache#5399 we need to clarify the intent of each IntegrationKit types.

* removal of imageWasKit
* usage of external IntegrationKit when we are certain that the image comes from an IK
* usage of syntetic IntegrationKit when the image are coming from any container image
* rework of promote command to clone an external IntegrationKit beside the Integration and use it accordingly.

Closes apache#5408

(cherry picked from commit apache/camel-k@aa6202c51)
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

4 participants