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

Ban use of '/' in config keys #420

Closed
kenfinnigan opened this issue Mar 18, 2019 · 13 comments
Closed

Ban use of '/' in config keys #420

kenfinnigan opened this issue Mar 18, 2019 · 13 comments
Assignees
Labels
clarification 🏆 A clarification (PR) or request for clarification (issue) in the spec or API
Milestone

Comments

@kenfinnigan
Copy link
Contributor

For "Cloud Native" use we need to ban the use of '/' within configuration keys.

As per eclipse/microprofile-rest-client#171, it's not possible to convert a configuration key with '/' into an appropriate environment variable.

I believe FT also uses '/' heavily in configuration keys.

@cescoffier
Copy link

I would even go further. The spec should enforce the IEEE 1003.1-2017 - IEEE Standard for IEEE Information Technology - (POSIX) recommendations.

Environment variable names used by the utilities in the Shell and Utilities volume of
POSIX.1-20xx consist solely of uppercase letters, digits, and the ’_’ (underscore). 

So / is not an accepted character.

I believe that all specs requiring / in their configuration key should be updated (as MP 3.0 allows breaking change, it's the right moment).

@Emily-Jiang
Copy link
Member

Config automatically converts non alphabetical numberic character to _ when trying to match environment variables. There is no issue when using /.

@cescoffier
Copy link

Doesn't that make environment variables weird with potentially several consecutive _.

@cescoffier
Copy link

And from eclipse/microprofile-rest-client#171 it seems that . is not replaced. While . should not be accepted either.

@cescoffier
Copy link

I'm actually confused because #264 says that . are replaced by _.

@cescoffier
Copy link

So basically, the spec should enforce that the mapped names are valid according to the IEEE recommendations.

@Emily-Jiang
Copy link
Member

As per my previous comment, all non alphabetical numberic will be replaced with _. It cover . / or any others, which is not alphabetical nor numberic.

@Emily-Jiang
Copy link
Member

Read the config spec, which explains more. I think there is any issue to provide wider replacements beyond . -> _

@kenfinnigan
Copy link
Contributor Author

For me it's not the fact that there isn't a solution around the problem, it's that we make it confusing by needing to know what the key can contain in different environments.

I would prefer the key is identical wherever it's used, which is why I think it makes sense for MP Config to ban inappropriate key name content

@cghislai
Copy link

i think it is not confusing. the intent and the behaviour are clearly specified, under the 'Default ConfigSources' title.
Other config sources might have other restrictions and require other key names mappings, so restricting it at the spec level would just put your issue one step further provided you use a non-default config source.

@hutchig
Copy link
Member

hutchig commented Jun 3, 2019

It would seem to me that if an organisation does not like '/' in properties it is free to do this as local policy and the Config spec will be perfectly happy with that too.

@hutchig
Copy link
Member

hutchig commented Jun 17, 2019

Also relevant is that https://github.com/eclipse/microprofile/pull/100/files references
FT's use of "/" as a delimeter as a common pattern!
Useful as a "/" indicates visually the end of a fully qualified classname.

@Emily-Jiang
Copy link
Member

At today's hangout, @kenfinnigan @jmesnil and I discussed this in more details. We should certainly document the character mapping in the javadoc of Config.java to ensure end users are aware of how to override properties in environment variables.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification 🏆 A clarification (PR) or request for clarification (issue) in the spec or API
Projects
None yet
Development

No branches or pull requests

6 participants