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

Properties from file system should be search with the same rules used for environment variables #276

Merged
merged 3 commits into from Apr 13, 2020

Conversation

kifj
Copy link
Contributor

@kifj kifj commented Apr 11, 2020

Statement of problem

given a interface for the MicroProfile REST client

package com.example;
@Produces(APPLICATION_JSON)
@RegisterRestClient
public interface QuickQuoteService {
  ...
}

which will retrieve its URL from MP Config with the key

com.example.QuickQuoteService/mp-rest/url

and a ConfigMap in Kubernetes with entries

kind: ConfigMap
data:
 com_example_QuickQuoteService_mp_rest_url: https://quote.cnbc.com/quote-html-webservice

A ConfigMap defines rules on the key, which forbids '/', because it cannot be mapped to an environment variable or a file name.

According to specification the replacement rules are defined for environment variables, all non-alphanumeric characters are replaced with '_'; and if that doesn't resolve, all alphabetical characters can be upper-case.

This ConfigMap will be mounted in a volume, resulting in a file named com_example_QuickQuoteService_mp_rest_url with the value as content.

The FileSystemConfigSource should pick up that file (which it does) and the Config should return its content as value for the key com.example.QuickQuoteService/mp-rest/url (which is doesn't)

Solution

The mapping rules for environment variables (see EnvConfigSource) should apply to file names as well, even if the specification does not define this behavior.

@radcortez
Copy link
Member

Hi @kifj

Thank yo for the PR!

Just a clarification: from my understanding, the spec only mandates the name mapping to happen for Environment Variables: https://github.com/eclipse/microprofile-config/blob/cafee0a79fd36dfaa4f7b30bc77efa75eefa589b/spec/src/main/asciidoc/configsources.asciidoc#L53

Now, that doesn't mean we shouldn't support this in the FileSystemConfigSource :)

Do you mind adding a few tests?

@radcortez radcortez self-requested a review April 13, 2020 09:57
@@ -96,4 +98,30 @@ private static String readContent(Path file) {
throw new UncheckedIOException(e);
}
}

@Override
public String getValue(String name) {
Copy link
Member

Choose a reason for hiding this comment

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

Add tests.

@kifj
Copy link
Contributor Author

kifj commented Apr 13, 2020

You're right regarding the specification so I have updated the comment and added a unit test

@kifj kifj requested a review from radcortez April 13, 2020 11:09
@radcortez
Copy link
Member

@radcortez radcortez changed the title Properties from file system should be search with the rewrite rules accoding to specification Properties from file system should be search with the same rules used for environment variables Apr 13, 2020
@radcortez radcortez merged commit 91b20f3 into smallrye:master Apr 13, 2020
@radcortez radcortez added this to the 1.7.1 milestone Apr 13, 2020
kifj added a commit to kifj/smallrye-config that referenced this pull request Jun 11, 2020
… for environment variables (smallrye#276)

Properties from file system should be search with the same rules used for environment variables.
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.

None yet

2 participants