feat(cmd/run): resource option refactoring #2355

squakez · 2021-06-03T13:45:03Z

With this PR we are refactoring the original --resource flag to behave similarly of what we've done with --config.

kamel run --resource [configmap|secret|file]:resourceName[/key][@/path/to/destination]

We are also refactoring the --config flag to include the possiblity to specify /key when using a secret/configmap.

The difference is that the --resource will allow the user to provide any kind of file (also binary files) and those files won't be considered as properties file by the Integration runtime.

Added support for Configmaps and Secrets
Mantained compatibility with --resource /path/to/file (though, it is marked as deprecated for future removal)
Support for @/path/ to let the user chose where the resource (file, secret or configmaps) will be mounted
Modeline support verified
File size limitation to 1MB introduced
Support for all local file synchronization
Possibility to filter a secret/configmap key both in --resource and --configmap
Updated CRDs to specify resources type (config or resource), resource key selection and destination path
Examples
E2E integration tests

Closes #2003
Closes #1831
Closes #1534
Closes #1533

Release Note

feat(cmd/run): resource option refactoring

* Provide support for configmap, secret or local file * Refactoring the structure of mount points to distinguish clearly between config and resource options Ref apache#2003

Ref apache#2003

Adding backward compatibility and a deprecation notice. Ref apache#2003

Ref apache#2003

Adding the possibility to specify the destination path for any resource Ref apache#2003

Ref apache#2003

Won't accept any resource/config file larger than 1MB, due to the limitation on K8S CustomResources Ref apache#2003

Ref apache#2003

* Added support to specify a single key from a configmap/secret in --resource or --config flag * Refactoring the RunConfigOption struct to include the new feature and hide the complexity to usage Ref apache#2003

Ref apache#2003

heiko-braun · 2021-06-08T15:12:44Z

Good work @squakez !

nicolaferraro · 2021-06-14T09:34:09Z

pkg/trait/util.go

+
+		for _, entry := range entries {
+			if entry.Type == configurationType {
+				var item = make(map[string]string)


Probably you could have used the entry struct type instead of map, but it's ok

nicolaferraro

Looks good, should we merge it?

squakez · 2021-06-14T10:39:12Z

Looks good, should we merge it?

Thanks for reviewing! Yes, you can proceed, I am already working on the documentation.

nicolaferraro · 2021-06-14T10:46:25Z

Looks good, should we merge it?

Thanks for reviewing! Yes, you can proceed, I am already working on the documentation.

I let @astefanutti do it since he said he was having a look..

astefanutti · 2021-06-14T10:48:09Z

Let me do a quick round of review 👍🏼.

astefanutti

Please find below a couple of points. These may rather be questions or comments, that can possibly be addressed subsequently:

I find it a bit hard to reason about the API, we have:
```
resources:
  compression
  content
  contentKey
  contentRef
  contentType
  mountPath
  name
  path
  rawContent
  type
```
and:
```
configuration:
  resourceKey
  resourceMountPoint
  resourceType
  type
  value
```
Do you think this could be improved, e.g., use more consistent name (mountPath vs resourceMountPoint), limit possible values with +kubebuilder:validation:Enum, add documentation, ...
For the option value, the format is [configmap|secret|file]:resourceName[/key]: I would find path instead of resourceName a bit more coherent between ConfigMap/Secret and file, mentioning that the path can be name/key for the former. Also, I wonder, should it be documented that the ConfigMap / Secret is implicitly located in the current namespace?
Could the --property option be merged with the --config option, to be symmetrical with the --resource option?
Should the documentation be updated according to the new CLI options: https://camel.apache.org/camel-k/latest/configuration/configmap-secret.html?
For the Integration container layout, I understand that the directories prefixed with _ are application configuration resources, and that this is inherited from the runtime, right? Should we mirror that layout for plain resources? I find adding that extra data directory not adding much meaning.

docs/modules/ROOT/pages/cli/modeline.adoc

squakez · 2021-06-15T13:22:29Z

Please find below a couple of points. These may rather be questions or comments, that can possibly be addressed subsequently:

* I find it a bit hard to reason about the API, we have:
  ```
  resources:
    compression
    content
    contentKey
    contentRef
    contentType
    mountPath
    name
    path
    rawContent
    type
  ```    
  and:
  ```
  configuration:
    resourceKey
    resourceMountPoint
    resourceType
    type
    value
  ```
  Do you think this could be improved, e.g., use more consistent name (`mountPath` vs `resourceMountPoint`), limit possible values with `+kubebuilder:validation:Enum`, add documentation, ...

Yeah, I had this part in mind as a refactoring tracked in #2320. If you're okey with the approach, we can review your comment as part of that development that I'm planning to do right after #2003 is closed.

* For the option value, the format is `[configmap|secret|file]:resourceName[/key]`: I would find `path` instead of `resourceName` a bit more coherent between ConfigMap/Secret and file, mentioning that the path can be `name/key` for the former. Also, I wonder, should it be documented that the ConfigMap / Secret is implicitly located in the _current_ namespace?

I see your point. I found difficult to show the syntax in the CLI as file won't have key. Any suggestion how to improve that is welcome. About the namespace, there are nice warnings now checking if a cm/secret is not found in the current namespace. The Integration is build, but the CLI will report it.

* Could the `--property` option be merged with the `--config` option, to be symmetrical with the `--resource` option?

I think they have a different meaning. The --property won't generate a file per se, but will be appended to a generated user.properties file. Both --config and --resource will be expected to generate a file on the Integration. A --config could be parsed by the runtime, whilst a --resource won't.

* Should the documentation be updated according to the new CLI options: https://camel.apache.org/camel-k/latest/configuration/configmap-secret.html?

Worry not, another PR is ready with examples and full documentation... I did not want to have this PR larger than it is now!

* For the Integration container layout, I understand that the directories prefixed with `_` are application configuration resources, and that this is inherited from the runtime, right? Should we mirror that layout for plain resources? I find adding that extra `data` directory not adding much meaning.

So, this would be the final tree expected on the Integration:

/etc/camel/sources --> will contain sources

/etc/camel/conf.d --> will contain configuration provided by user
/etc/camel/conf.d/_resources/ --> --config file:
/etc/camel/conf.d/_secrets/ --> --config secret:
/etc/camel/conf.d/_configmaps/ --> --config configmap:   
            
/etc/camel/data --> default location for data provided by the user 
/etc/camel/data/resources/ --> --resource file:
/etc/camel/data/secrets/ --> --resource secret:
/etc/camel/data/configmaps/ --> --resource configmap:

The conf.d is something the final user should ignore at all and it's what is expected by the runtime logic. In fact, I added a validation to avoid any user tampering those directories when using the path feature. The data directory is something that the user may be able to use in his integrations, as this is the default location when the path is not specified in the --resource option. Now, I thought that data can be easily understood by the audience, as well as using an _ in the path may be cumbersome. The new structure will be also explained in the documentation. We can think to have a different directory for what is now called data, I don't have preferences, however, I'd avoid the usage of _.

Ref apache#2003

astefanutti · 2021-06-15T13:53:27Z

Please find below a couple of points. These may rather be questions or comments, that can possibly be addressed subsequently:
* I find it a bit hard to reason about the API, we have:
resources:
compression
content
contentKey
contentRef
contentType
mountPath
name
path
rawContent
type
and:
configuration:
resourceKey
resourceMountPoint
resourceType
type
value
Do you think this could be improved, e.g., use more consistent name (`mountPath` vs `resourceMountPoint`), limit possible values with `+kubebuilder:validation:Enum`, add documentation, ...
Yeah, I had this part in mind as a refactoring tracked in #2320. If you're okey with the approach, we can review your comment as part of that development that I'm planning to do right after #2003 is closed.

Great, sounds good. The sooner the better to avoid dealing with backward compatibility.

* For the option value, the format is `[configmap|secret|file]:resourceName[/key]`: I would find `path` instead of `resourceName` a bit more coherent between ConfigMap/Secret and file, mentioning that the path can be `name/key` for the former. Also, I wonder, should it be documented that the ConfigMap / Secret is implicitly located in the _current_ namespace?
I see your point. I found difficult to show the syntax in the CLI as file won't have key. Any suggestion how to improve that is welcome. About the namespace, there are nice warnings now checking if a cm/secret is not found in the current namespace. The Integration is build, but the CLI will report it.

I cannot think of a one-size-fits-all solution, but the following may be other ways:

[configmap|secret|file]:path, where path can be name/key for ConfigMap / Secret
[configmap|secret|file]:nameOrPath[/key]

* Could the `--property` option be merged with the `--config` option, to be symmetrical with the `--resource` option?
I think they have a different meaning. The --property won't generate a file per se, but will be appended to a generated user.properties file. Both --config and --resource will be expected to generate a file on the Integration. A --config could be parsed by the runtime, whilst a --resource won't.

Ah right. The --property file: syntax seems to overlap the --config file.

* Should the documentation be updated according to the new CLI options: https://camel.apache.org/camel-k/latest/configuration/configmap-secret.html?
Worry not, another PR is ready with examples and full documentation... I did not want to have this PR larger than it is now!

👍🏼

* For the Integration container layout, I understand that the directories prefixed with `_` are application configuration resources, and that this is inherited from the runtime, right? Should we mirror that layout for plain resources? I find adding that extra `data` directory not adding much meaning.
So, this would be the final tree expected on the Integration:
/etc/camel/sources --> will contain sources

/etc/camel/conf.d --> will contain configuration provided by user
/etc/camel/conf.d/_resources/ --> --config file:
/etc/camel/conf.d/_secrets/ --> --config secret:
/etc/camel/conf.d/_configmaps/ --> --config configmap:   
            
/etc/camel/data --> default location for data provided by the user 
/etc/camel/data/resources/ --> --resource file:
/etc/camel/data/secrets/ --> --resource secret:
/etc/camel/data/configmaps/ --> --resource configmap:
The conf.d is something the final user should ignore at all and it's what is expected by the runtime logic. In fact, I added a validation to avoid any user tampering those directories when using the path feature. The data directory is something that the user may be able to use in his integrations, as this is the default location when the path is not specified in the --resource option. Now, I thought that data can be easily understood by the audience, as well as using an _ in the path may be cumbersome. The new structure will be also explained in the documentation. We can think to have a different directory for what is now called data, I don't have preferences, however, I'd avoid the usage of _.

Actually, I would rather get rid of the _ also in conf.d, but my understanding is that it requires changes into the runtime, does it?

For data, I was thinking to get rid of that extra level, that is /etc/camel/resources, /etc/camel/configmaps, ... I think even having a single /etc/camel/resources directory would be better. The likelihood of having conflicts is seldom.

Ref apache#2003

squakez · 2021-06-16T10:23:28Z

@astefanutti I've just committed 20b770c to include a clearer option description and to use a single resource directory. If we merge this, I will provide a newer PR with the full documentation. Thanks!

astefanutti · 2021-06-16T12:04:42Z

@squakez great, thanks. Let's merge it!

squakez added 14 commits June 3, 2021 11:29

feat(cmd/run): resource option refactoring

b653bd8

* Provide support for configmap, secret or local file * Refactoring the structure of mount points to distinguish clearly between config and resource options Ref apache#2003

refactor(crd): configuration resource type

e37a17d

Ref apache#2003

refactor(example): resource run option

2b478ad

refactor(e2e): resource option integration test

860baf6

Ref apache#2003

feat(cmd/run): --resource filename compatibility

ca53f32

Adding backward compatibility and a deprecation notice. Ref apache#2003

feat(cmd/run): modeline resource support

4febc7d

Ref apache#2003

feat(cmd/run): resource @path support

3eec946

Adding the possibility to specify the destination path for any resource Ref apache#2003

chore(crd): autogen update

0f4866f

Ref apache#2003

doc(example): resource @path

54e6a60

feat(cmd/run): file size limitation

38f70a0

Won't accept any resource/config file larger than 1MB, due to the limitation on K8S CustomResources Ref apache#2003

refactor(cmd/run): synch all option files

14a940f

Ref apache#2003

chore(e2e): run option with destination path

55f970d

Ref apache#2003

feat(cmd/run): validate destination path

35b5eac

Ref apache#2003

fix(e2e): test name typo

5d895aa

oscerd approved these changes Jun 4, 2021

View reviewed changes

squakez added 4 commits June 7, 2021 12:37

feat(cmd/run): configmap/secret key filtering

a81e935

* Added support to specify a single key from a configmap/secret in --resource or --config flag * Refactoring the RunConfigOption struct to include the new feature and hide the complexity to usage Ref apache#2003

chore(crd): configmap/secret eky filtering regen

1e2ce2f

Ref apache#2003

chore(e2e): configmap/secret key test

128a97b

Ref apache#2003

doc(example): configmap/secret key selection

d851246

nicolaferraro reviewed Jun 14, 2021

View reviewed changes

nicolaferraro approved these changes Jun 14, 2021

View reviewed changes

astefanutti reviewed Jun 15, 2021

View reviewed changes

docs/modules/ROOT/pages/cli/modeline.adoc Outdated Show resolved Hide resolved

docs/modules/ROOT/pages/cli/modeline.adoc Outdated Show resolved Hide resolved

feat(cmd/run): warn when using path on --config

bb94003

Ref apache#2003

squakez force-pushed the feat/2003_resource branch from 8b3298c to bb94003 Compare June 15, 2021 13:28

astefanutti added area/cli Kamel CLI area/core Core features of the integration platform labels Jun 15, 2021

chore(cmd/run): single resource dir and cli syntax description

20b770c

Ref apache#2003

astefanutti merged commit 73339cf into apache:main Jun 16, 2021

squakez mentioned this pull request Jun 16, 2021

feat(cmd/run): resource option refactoring jboss-fuse/camel-k#39

Merged

nicolaferraro mentioned this pull request Jul 2, 2021

Release 1.5.0 #2470

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cmd/run): resource option refactoring #2355

feat(cmd/run): resource option refactoring #2355

squakez commented Jun 3, 2021 •

edited

Loading

heiko-braun commented Jun 8, 2021

nicolaferraro Jun 14, 2021

nicolaferraro left a comment

squakez commented Jun 14, 2021

nicolaferraro commented Jun 14, 2021

astefanutti commented Jun 14, 2021

astefanutti left a comment

squakez commented Jun 15, 2021

astefanutti commented Jun 15, 2021

squakez commented Jun 16, 2021

astefanutti commented Jun 16, 2021

feat(cmd/run): resource option refactoring #2355

feat(cmd/run): resource option refactoring #2355

Conversation

squakez commented Jun 3, 2021 • edited Loading

heiko-braun commented Jun 8, 2021

nicolaferraro Jun 14, 2021

Choose a reason for hiding this comment

nicolaferraro left a comment

Choose a reason for hiding this comment

squakez commented Jun 14, 2021

nicolaferraro commented Jun 14, 2021

astefanutti commented Jun 14, 2021

astefanutti left a comment

Choose a reason for hiding this comment

squakez commented Jun 15, 2021

astefanutti commented Jun 15, 2021

squakez commented Jun 16, 2021

astefanutti commented Jun 16, 2021

squakez commented Jun 3, 2021 •

edited

Loading