You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by GitBox <gi...@apache.org> on 2021/07/22 08:53:18 UTC
[GitHub] [camel-k] squakez opened a new pull request #2519: [discussion] feat: configuration trait
squakez opened a new pull request #2519:
URL: https://github.com/apache/camel-k/pull/2519
<!-- Description -->
This draft PR serves as an helper to discuss the changes required to develop feature #2320.
1. Right now a property file or other resources attached to an `Integration`, are materialized via `deployment`, `cron` or `knative_service` traits. If we introduce the `configuration` trait we can let this take care of this aspect, although I am not entirely sure if this will have some drawback I am not able to see at the moment (see the changes proposed). It basically means moving the logic that is taking care of it (`e.computeConfigmaps()`) from the 3 trait into the other one. We are introducing some trait dependency (to `configuration` trait), but I don't see this a big concern.
2. If we're happy with point 1, then we'll need to start moving the logic that is happening now in the `e.computeConfigmaps()` directly in the configuration trait. As an example, in this PR you will see I've done this with the `Properties` parameter which is going to take care of the user properties.
3. What was done here with `Properties` will have to be extended with all the other user configuration such as `config`, `resource`, ... It seems doable with no big concerns either.
4. `ConfigurationSpec` is widely used in `Integration`, `IntegrationKit` and only in one case in `IntegrationPlatform`. If we proceed with this design change we may be in the situation to extend the trait to the kit as well. Or we can keep it in the kit and the platform and just remove from the integration spec only.
5. The backward compatibility is something we will have to take case as well. I think we can find some way to let both approach (`integration.spec.configspec` and `configuration` trait) coexist, deprecating the first and eventually remove it.
It does not look an easy development as there are several dependencies to the `ConfigurationSpec`. In any case, with the approach proposed we will be able to have all in a unique place, although it means we will depends on a trait. With the changes proposed here I manage to make an integration work seamlessly.
@astefanutti, @lburgazzoli you were involved in the past in this discussion, I'll gladly receive your feedback to confirm if the approach is fine and it's worth to keep ahead with this.
CC @nicolaferraro @oscerd @doru1004 @jamesnetherton @johnpoth @tadayosi @orpiske @davsclaus, happy if you can have a look as well.
<!--
Enter your extended release note in the below block. If the PR requires
additional action from users switching to the new release, include the string
"action required". If no release note is required, write "NONE".
You can (optionally) mark this PR with labels "kind/bug" or "kind/feature" to make sure
the text is added to the right section of the release notes.
-->
**Release Note**
```release-note
NONE
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] lburgazzoli commented on pull request #2519: [discussion] feat: configuration trait
Posted by GitBox <gi...@apache.org>.
lburgazzoli commented on pull request #2519:
URL: https://github.com/apache/camel-k/pull/2519#issuecomment-884811926
> 1. Right now a property file or other resources attached to an `Integration`, are materialized via `deployment`, `cron` or `knative_service` traits. If we introduce the `configuration` trait we can let this take care of this aspect, although I am not entirely sure if this will have some drawback I am not able to see at the moment (see the changes proposed). It basically means moving the logic that is taking care of it (`e.computeConfigmaps()`) from the 3 trait into the other one. We are introducing some trait dependency (to `configuration` trait), but I don't see this a big concern.
>
> 2. If we're happy with point 1, then we'll need to start moving the logic that is happening now in the `e.computeConfigmaps()` directly in the configuration trait. As an example, in this PR you will see I've done this with the `Properties` parameter which is going to take care of the user properties.
>
> 3. What was done here with `Properties` will have to be extended with all the other user configuration such as `config`, `resource`, ... It seems doable with no big concerns either.
I think that implementing those points is a good opportunity to cleanup the code and make it more consistent so +1 with me
> 4. `ConfigurationSpec` is widely used in `Integration`, `IntegrationKit` and only in one case in `IntegrationPlatform`. If we proceed with this design change we may be in the situation to extend the trait to the kit as well. Or we can keep it in the kit and the platform and just remove from the integration spec only.
This point is not super clear to me, in particular the reason to remove the trait from the integration.
> 5. The backward compatibility is something we will have to take case as well. I think we can find some way to let both approach (`integration.spec.configspec` and `configuration` trait) coexist, deprecating the first and eventually remove it.
I think that trait.Configure for the configuration trait, should by default attempt to converting the `.spec.config` to its own structure so we can support the two mode for some time but we would swich to the trait internally
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] squakez commented on pull request #2519: [discussion] feat: configuration trait
Posted by GitBox <gi...@apache.org>.
squakez commented on pull request #2519:
URL: https://github.com/apache/camel-k/pull/2519#issuecomment-920009080
Superseded by #2635
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] squakez closed pull request #2519: [discussion] feat: configuration trait
Posted by GitBox <gi...@apache.org>.
squakez closed pull request #2519:
URL: https://github.com/apache/camel-k/pull/2519
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] orpiske commented on pull request #2519: [discussion] feat: configuration trait
Posted by GitBox <gi...@apache.org>.
orpiske commented on pull request #2519:
URL: https://github.com/apache/camel-k/pull/2519#issuecomment-885448324
I think those are good points and I don't really have anything to add to them right now. I do have a comment about the following:
> > ```
> > 5. The backward compatibility is something we will have to take case as well. I think we can find some way to let both approach (`integration.spec.configspec` and `configuration` trait) coexist, deprecating the first and eventually remove it.
> > ```
>
> I think that trait.Configure for the configuration trait, should by default attempt to converting the `.spec.config` to its own structure so we can support the two mode for some time but we would swich to the trait internally
+1. I don't really worry with the specifics of the technical implementation at this point, but I think we should aim for a solution that allows seamless migration from one to another from the user perspective.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] squakez commented on pull request #2519: [discussion] feat: configuration trait
Posted by GitBox <gi...@apache.org>.
squakez commented on pull request #2519:
URL: https://github.com/apache/camel-k/pull/2519#issuecomment-885439304
Thanks Luca, some clarification inline:
> > ```
> > 1. Right now a property file or other resources attached to an `Integration`, are materialized via `deployment`, `cron` or `knative_service` traits. If we introduce the `configuration` trait we can let this take care of this aspect, although I am not entirely sure if this will have some drawback I am not able to see at the moment (see the changes proposed). It basically means moving the logic that is taking care of it (`e.computeConfigmaps()`) from the 3 trait into the other one. We are introducing some trait dependency (to `configuration` trait), but I don't see this a big concern.
> >
> > 2. If we're happy with point 1, then we'll need to start moving the logic that is happening now in the `e.computeConfigmaps()` directly in the configuration trait. As an example, in this PR you will see I've done this with the `Properties` parameter which is going to take care of the user properties.
> >
> > 3. What was done here with `Properties` will have to be extended with all the other user configuration such as `config`, `resource`, ... It seems doable with no big concerns either.
> > ```
>
> I think that implementing those points is a good opportunity to cleanup the code and make it more consistent so +1 with me
>
> > ```
> > 4. `ConfigurationSpec` is widely used in `Integration`, `IntegrationKit` and only in one case in `IntegrationPlatform`. If we proceed with this design change we may be in the situation to extend the trait to the kit as well. Or we can keep it in the kit and the platform and just remove from the integration spec only.
> > ```
>
> This point is not super clear to me, in particular the reason to remove the trait from the integration.
I did not explain properly. I was meaning that, as soon as we go deep in the development, we may be in the opportunity to apply the same mechanics used in the new `configuration` trait also for the `ConfigurationSpec` now hold by the `IntegrationKit`. In any case it's too early to say. I will come back on this point if it needs further discussion.
>
> > ```
> > 5. The backward compatibility is something we will have to take case as well. I think we can find some way to let both approach (`integration.spec.configspec` and `configuration` trait) coexist, deprecating the first and eventually remove it.
> > ```
>
> I think that trait.Configure for the configuration trait, should by default attempt to converting the `.spec.config` to its own structure so we can support the two mode for some time but we would swich to the trait internally
It's a good approach. I was thinking about maintaining temporarily the repeated logic (ie, e.computeConfigmaps() + the new logic), though I don't like repeated code. I will apply your suggestion as it looks cleaner.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org