astefanutti commented on pull request #2355: URL: https://github.com/apache/camel-k/pull/2355#issuecomment-861518206
> > 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. -- 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: us...@infra.apache.org
