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

CHE-450 do not fail if impossible to resolve variable in environment props #1132

Merged
merged 1 commit into from
Apr 27, 2016

Conversation

mshaposhnik
Copy link
Contributor

@mshaposhnik mshaposhnik commented Apr 26, 2016

Signed-off-by: Max Shaposhnik mshaposhnik@codenvy.com

@mshaposhnik mshaposhnik changed the title CHE-450 do not fail if impossible to resolve variable in system and e… CHE-450 do not fail if impossible to resolve variable in environment props Apr 26, 2016
@@ -239,11 +243,16 @@ protected void bindProperties(String prefix, Map<String, String> properties) {
if (actual == null) {
actual = System.getenv(name);
}
if (actual == null) {
if (!skipUnresolved && actual == null) {
Copy link
Member

Choose a reason for hiding this comment

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

We can rewrite it to avoid double check actual value

if (actual != null) {
    buf.append(actual);
} else if (skipUnresolved) {
    buf.append(pValue.substring(i, j));
    LOG.warn("Environment variable " + name + " cannot be resolved, leaving as is.");
} else {
    throw new ConfigurationException(...);
}

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

@skabashnyuk
Copy link
Contributor

ok @garagatyi ?

@@ -239,11 +243,16 @@ protected void bindProperties(String prefix, Map<String, String> properties) {
if (actual == null) {
actual = System.getenv(name);
}
if (actual == null) {
if (actual != null) {

Choose a reason for hiding this comment

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

you can use else instead of if (actual != null)

Copy link
Member

@sleshchenko sleshchenko Apr 26, 2016

Choose a reason for hiding this comment

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

I think I will have +1 level nesting in your case. Can you provide full example?

Choose a reason for hiding this comment

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

Sorry, you can't

@garagatyi
Copy link

LGTM

@mshaposhnik mshaposhnik merged commit ac3c8b9 into master Apr 27, 2016
@mshaposhnik mshaposhnik deleted the CHE-450 branch April 27, 2016 07:35
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.

4 participants