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(trait): camel trait to include properties #2635

Merged
merged 6 commits into from
Sep 28, 2021

Conversation

squakez
Copy link
Contributor

@squakez squakez commented Sep 15, 2021

Previously drafted in #2519, with this PR we're putting properties management into camel trait. We're moving the actual configuration that was used in deployment, cron and knative_service that was taking care of computing configmaps related to camel properties, sources and resources.

Ref #2320

Release Note

feat(trait): camel trait to include properties

@squakez squakez force-pushed the feat/2320 branch 2 times, most recently from 0e6430e to 231ac42 Compare September 15, 2021 14:46
Copy link
Member

@astefanutti astefanutti left a comment

Choose a reason for hiding this comment

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

I wonder what do you see should be added as parameters to that new configuration trait? I'm asking just to understand what responsibilities should be captured by that new trait, as configuration is a very general concept. So I wonder, if that's going to be application properties and resources, maybe a new runtime trait may be the right abstraction that we are trying to capture, as we have other traits, like camel, builder_, deployer, platform (pretty much every trait) that "configure" things to some extend.

@squakez
Copy link
Contributor Author

squakez commented Sep 16, 2021

I wonder what do you see should be added as parameters to that new configuration trait?

In the immediate term, we should have properties, resources and config (which logic is actually behind kamel run --p/--resource/--config).

I'm asking just to understand what responsibilities should be captured by that new trait, as configuration is a very general concept. So I wonder, if that's going to be application properties and resources, maybe a new runtime trait may be the right abstraction that we are trying to capture, as we have other traits, like camel, builder_, deployer, platform (pretty much every trait) that "configure" things to some extend.

I don't have a strong opinion on the name to adopt here. The original discussion was born when we started the refactoring of the user properties and configuration. Considering that the idea is also to replace .spec.configuration I felt it was consistent, although very generic name.

@squakez
Copy link
Contributor Author

squakez commented Sep 16, 2021

Adding another option to think about I can see also integration may fit in.

@astefanutti
Copy link
Member

Adding another option to think about I can see also integration may fit in.

It's definitely more tangible than "configuration", but it may be still quite too general. "Integration" is a lot of things. I think we try to follow the single responsibility principle as much as possible for traits, and avoid overlapping across traits.

So we would have properties, resources and config. My understanding is that properties are application / runtime properties. Looking at the existing description of the --resources and --config options, runtime is also mentioned:

cmd.Flags().StringArray("config", nil, "Add a runtime configuration from a Configmap, a Secret or a file (syntax: [configmap|secret|file]:name[/key], where name represents the local file path or the configmap/secret name and key optionally represents the configmap/secret key to be filtered)")
cmd.Flags().StringArray("resource", nil, "Add a runtime resource from a Configmap, a Secret or a file (syntax: [configmap|secret|file]:name[/key][@path], where name represents the local file path or the configmap/secret name, key optionally represents the configmap/secret key to be filtered and path represents the destination path)")

So from the functional standpoint, for users, it would be about configuring the runtime. If we take the technical standpoint (that may be less relevant for the end-users), these configuration bits are ConfigMap or Secret mounted into the Integration Pod(s), so the pod or container traits may already be responsible for related concerns.

@astefanutti
Copy link
Member

astefanutti commented Sep 16, 2021

I don't have a strong opinion on the name to adopt here. The original discussion was born when we started the refactoring of the user properties and configuration. Considering that the idea is also to replace .spec.configuration I felt it was consistent, although very generic name.

Quoting #2003, "As today there is a little bit of confusion about the options to configure the runtime", it seems runtime configuration was the original intent of these .spec.configuration fields. I think it's worth iterating on the naming and avoid repeating history :)

@squakez
Copy link
Contributor Author

squakez commented Sep 16, 2021

I rest my case... 😆

all good arguments to chose runtime after all. I'll change it shortly.

@astefanutti
Copy link
Member

astefanutti commented Sep 16, 2021

I rest my case... 😆

all good arguments to chose runtime after all. I'll change it shortly.

LOL, I realise that was "runtime, runtime, bla, runtime...". I did not want to give the impression of pushing a particular term. I though about it, and the more I've dug, the more "runtime" I've found :) I'm not sure if that correlates with good decision making. We can wait a bit to gather more people feedback, even if a consensus / voting algorithm do not necessarily correlates with good decision making neither when it comes to naming things :)

@astefanutti
Copy link
Member

One thing that bugs me about a runtime trait is that we have the camel trait that has a runtimeVersion property, and that is somehow about configuring the runtime. I don't think any users override the camel trait, so an idea would be to merge it into that new runtime trait?

@squakez
Copy link
Contributor Author

squakez commented Sep 16, 2021

One thing that bugs me about a runtime trait is that we have the camel trait that has a runtimeVersion property, and that is somehow about configuring the runtime. I don't think any users override the camel trait, so an idea would be to merge it into that new runtime trait?

I think it's a good idea. I'll add a follow up PR to take care of it.

@squakez squakez added the status/wip Work in progress label Sep 16, 2021
@astefanutti
Copy link
Member

astefanutti commented Sep 20, 2021

Alternatively, an idea would be to keep the camel trait, and add these configuration options to it. These are about configuring the Camel runtime after all, and it may be more appealing to users than a more abstract runtime name.

@squakez
Copy link
Contributor Author

squakez commented Sep 20, 2021

Alternatively, an idea would be to keep the camel trait, and add these configuration options to it. These are about configuring the Camel runtime after all, and it may be more appealing to users than a more abstract runtime name.

I've progressed more deeply and I finally realized that the trait that should be in charge of them would be container. Basically, when it comes to config and resource there is the need to attach a volume, and this is done in container trait. However there is some incompatibility, see #2320 (comment) for more details.

@squakez squakez marked this pull request as draft September 20, 2021 08:20
@astefanutti
Copy link
Member

astefanutti commented Sep 20, 2021

Alternatively, an idea would be to keep the camel trait, and add these configuration options to it. These are about configuring the Camel runtime after all, and it may be more appealing to users than a more abstract runtime name.

I've progressed more deeply and I finally realized that the trait that should be in charge of them would be container. Basically, when it comes to config and resource there is the need to attach a volume, and this is done in container trait. However there is some incompatibility, see #2320 (comment) for more details.

Yes, that was one of my comment above, if we take the technical standpoint, these configuration bits are ConfigMap or Secret mounted into the Integration Pod(s), so the pod or container traits may already be responsible for related concerns. If we don't find a functional standpoint that works and that would be more relevant to users, we can fallback to this approach (even the container trait is already quite crowded). Following that path, the properties would go into the camel trait?

@squakez
Copy link
Contributor Author

squakez commented Sep 20, 2021

Alternatively, an idea would be to keep the camel trait, and add these configuration options to it. These are about configuring the Camel runtime after all, and it may be more appealing to users than a more abstract runtime name.

I've progressed more deeply and I finally realized that the trait that should be in charge of them would be container. Basically, when it comes to config and resource there is the need to attach a volume, and this is done in container trait. However there is some incompatibility, see #2320 (comment) for more details.

Yes, that was one of my comment above, if we take the technical standpoint, these configuration bits are ConfigMap or Secret mounted into the Integration Pod(s), so the pod or container traits may already be responsible for related concerns. If we don't find a functional standpoint that works and that would be more relevant to users, we can fallback to this approach (even the container trait is already quite crowed). Then, the properties would go to into the camel trait?

Yes, that's correct. The properties would be a perfect fit for camel trait. I was more thinking on a general refactoring for all Integration configuration. Let's figure out what we want to do for all the cases first, and I will retake this one later.

@squakez squakez marked this pull request as ready for review September 24, 2021 10:41
@squakez squakez removed the status/wip Work in progress label Sep 24, 2021
@squakez squakez changed the title feat(trait): configuration base trait feat(trait): camel trait to include properties Sep 24, 2021
@squakez
Copy link
Contributor Author

squakez commented Sep 27, 2021

@astefanutti I've moved the logic into camel trait if you want to give a further look. After all it made all sense to go in that direction.

pkg/trait/camel.go Outdated Show resolved Hide resolved
docs/modules/traits/pages/configuration.adoc Outdated Show resolved Hide resolved
docs/modules/traits/pages/runtime.adoc Outdated Show resolved Hide resolved
* 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
Co-authored-by: Antonin Stefanutti <[email protected]>
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