squakez commented on pull request #2355: URL: https://github.com/apache/camel-k/pull/2355#issuecomment-861493512
> 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 `_`. -- 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. For queries about this service, please contact Infrastructure at: [email protected]
