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

[Trait] Configuration refactoring #2320

Closed
squakez opened this issue May 24, 2021 · 6 comments · Fixed by #2771
Closed

[Trait] Configuration refactoring #2320

squakez opened this issue May 24, 2021 · 6 comments · Fixed by #2771
Assignees
Labels
kind/feature New feature or request status/wip Work in progress

Comments

@squakez
Copy link
Contributor

squakez commented May 24, 2021

As commented in #2003, we should introduce a new Configuration trait in order to properly encapsulate the logic of the different configurations provided into an Integration. This new capability would simplify the configuration of traits as also expressed in #2165.

Once developed we can deprecate .spec.configuration.

I am tracking this as a spin off issue to be worked after finishing #2003, as the latter will aim to remove configurations that may be part of this new trait.

@squakez
Copy link
Contributor Author

squakez commented May 24, 2021

I will work on this after #2003

@squakez squakez self-assigned this Aug 9, 2021
@squakez squakez added the kind/feature New feature or request label Aug 10, 2021
@squakez squakez added the status/wip Work in progress label Sep 13, 2021
squakez added a commit to squakez/camel-k that referenced this issue Sep 15, 2021
Moving the logic previously defined in deployment, cron and knative service into a configuration trait

Ref apache#2320
squakez added a commit to squakez/camel-k that referenced this issue Sep 15, 2021
@squakez
Copy link
Contributor Author

squakez commented Sep 20, 2021

I've analyzed this more deeply. It is possible to think a trait, probably container trait, to handle --config and --resource, moving the parsing logic directly in the trait (which is also in charge to mount the volumes, so it even simplify the "modularity").

However we have a big limitation when it comes to file usage. The file content must be stored within the same Integration, so that it can be transformed on the fly into a configmap and used. The trait is executed by the operator and it won't have access to the file content (unless we store it previously, as we're doing now).

One possibility would be to get rid of file. After all, the user should have familiarity with cloud native way of storing resources (ie, configmap) and create them beside the integration. The same would apply to --openapi and would deprecate .integration.spec.resources as well.

@astefanutti @nicolaferraro @lburgazzoli wdyt?

@astefanutti
Copy link
Member

One possibility would be to get rid of file. After all, the user should have familiarity with cloud native way of storing resources (ie, configmap) and create them beside the integration. The same would apply to --openapi and would deprecate .integration.spec.resources as well.

Right. Alternatively, the kamel run CLI could be responsible for creating the ConfigMap corresponding to the file: config option. to avoid that two-stepped experience. Anyway, I agree we should avoid storing arbitrary files into the Integration resources.

@squakez
Copy link
Contributor Author

squakez commented Sep 20, 2021

One possibility would be to get rid of file. After all, the user should have familiarity with cloud native way of storing resources (ie, configmap) and create them beside the integration. The same would apply to --openapi and would deprecate .integration.spec.resources as well.

Right. Alternatively, the kamel run CLI could be responsible for creating the ConfigMap corresponding to the file: config option. to avoid that two-stepped experience. Anyway, I agree we should avoid storing arbitrary files into the Integration resources.

Yeah, I thought the same, but we even complicate the process as we need to parse twice. The first time in the cmd in order to process the content, the second time in the trait as we need to locate the resource. Moreover, I am not sure if we would actually able to scale or recreate the Pod properly. Right now we leverage the presence of the content in the Integration, if we create an attached configmap we will have to make sure this is not deleted unless the user is deliberately deleting the Integration. I think we can enter in a dangerous path.

@astefanutti
Copy link
Member

One possibility would be to get rid of file. After all, the user should have familiarity with cloud native way of storing resources (ie, configmap) and create them beside the integration. The same would apply to --openapi and would deprecate .integration.spec.resources as well.

Right. Alternatively, the kamel run CLI could be responsible for creating the ConfigMap corresponding to the file: config option. to avoid that two-stepped experience. Anyway, I agree we should avoid storing arbitrary files into the Integration resources.

Yeah, I thought the same, but we even complicate the process as we need to parse twice. The first time in the cmd in order to process the content, the second time in the trait as we need to locate the resource. Moreover, I am not sure if we would actually able to scale or recreate the Pod properly. Right now we leverage the presence of the content in the Integration, if we create an attached configmap we will have to make sure this is not deleted unless the user is deliberately deleting the Integration. I think we can enter in a dangerous path.

You're right, but I think Camel K should not concern about it. This should be delegated to the underlying Kubernetes controller, like the Deployment controller, as much as possible. For example, if the user deletes the ConfigMap, the Deployment controller will handle it. For the use case of responding to changes, it's also a vast area with long standing issues like kubernetes/kubernetes#22368.

squakez added a commit to squakez/camel-k that referenced this issue Sep 24, 2021
Moving the logic previously defined in deployment, cron and knative service into a configuration trait

Ref apache#2320
squakez added a commit to squakez/camel-k that referenced this issue Sep 24, 2021
squakez added a commit to squakez/camel-k that referenced this issue Sep 24, 2021
* Camel trait is the most suitable to control how properties are managed
* Some refactoring in trait_test.go as the properties configmap is now managed by camel trait

Closes apache#2320
squakez added a commit to squakez/camel-k that referenced this issue Sep 24, 2021
* Camel trait is the most suitable to control how properties are managed
* Some refactoring in trait_test.go as the properties configmap is now managed by camel trait

Closes apache#2320
squakez added a commit to squakez/camel-k that referenced this issue Sep 27, 2021
* Camel trait is the most suitable to control how properties are managed
* Some refactoring in trait_test.go as the properties configmap is now managed by camel trait

Closes apache#2320
astefanutti pushed a commit that referenced this issue Sep 28, 2021
Moving the logic previously defined in deployment, cron and knative service into a configuration trait

Ref #2320
astefanutti pushed a commit that referenced this issue Sep 28, 2021
@squakez
Copy link
Contributor Author

squakez commented Sep 28, 2021

The previous commits were relating this issue, but not closing it.

@squakez squakez reopened this Sep 28, 2021
@squakez squakez changed the title [Trait] Configuration refactorying [Trait] Configuration refactoring Nov 23, 2021
squakez added a commit to squakez/camel-k that referenced this issue Nov 23, 2021
squakez added a commit to squakez/camel-k that referenced this issue Nov 23, 2021
squakez added a commit to squakez/camel-k that referenced this issue Nov 24, 2021
squakez added a commit to squakez/camel-k that referenced this issue Nov 25, 2021
squakez added a commit to squakez/camel-k that referenced this issue Nov 25, 2021
squakez added a commit to squakez/camel-k that referenced this issue Nov 25, 2021
squakez added a commit to squakez/camel-k that referenced this issue Dec 15, 2021
squakez added a commit to squakez/camel-k that referenced this issue Dec 15, 2021
squakez added a commit to squakez/camel-k that referenced this issue Dec 17, 2021
squakez added a commit to squakez/camel-k that referenced this issue Dec 17, 2021
squakez added a commit that referenced this issue Jan 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature New feature or request status/wip Work in progress
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants