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

Minor YAML config improvements #15389

Merged
merged 2 commits into from Aug 15, 2019

Conversation

@blazember
Copy link
Contributor

blazember commented Jul 29, 2019

  • Make hazelcast-specific root-nodes optional
  • Replace usages of locateFromSystemPropertyOrFailOnUnacceptedSuffix with locateFromSystemProperty in config resolution methods, since it was confusing
  • Align comments with the actual behavior
  • Update tests with the changes
config = new XmlClientFailoverConfigBuilder(xmlConfigLocator).build();

} else if (yamlConfigLocator.locateFromSystemPropertyOrFailOnUnacceptedSuffix()) {

This comment has been minimized.

Copy link
@blazember

blazember Jul 29, 2019

Author Contributor

It can only be an accepted YAML suffix here. See the validateSuffixInSystemProperty a few lines above. It was validated for XML and YAML suffixes, and it is not XML.

@blazember

This comment has been minimized.

Copy link
Contributor Author

blazember commented Jul 29, 2019

@vojtechtoman vojtechtoman self-requested a review Aug 14, 2019
@petrpleshachkov petrpleshachkov self-requested a review Aug 14, 2019

buildConfig(yaml);
ClientConfig clientConfig = buildConfig(yaml);
assertNotEquals("my-instance", clientConfig.getInstanceName());

This comment has been minimized.

Copy link
@petrpleshachkov

petrpleshachkov Aug 14, 2019

Contributor

Minor: may it would be better to check which value is expected here?

This comment has been minimized.

Copy link
@blazember

blazember Aug 14, 2019

Author Contributor

It would be better, I change this to assertNull instead, since in this case instanceName is not expected to be set.

This comment has been minimized.

Copy link
@blazember

blazember Aug 15, 2019

Author Contributor

Done

blazember added 2 commits Jul 26, 2019
- Make hazelcast-specific root-nodes optional
- Remove usages of locateFromSystemPropertyOrFailOnUnacceptedSuffix
config resolution codes, since it is confusing
- Align comments with the actual behavior
- Update tests with the changes
@blazember blazember force-pushed the blazember:4.0/yaml/minor-improvements branch from cd0c288 to 66b70aa Aug 15, 2019
@@ -108,6 +110,18 @@ public void testExplicitNullScalarThrows() {
buildConfig(yaml);
}

@Test

This comment has been minimized.

Copy link
@vojtechtoman

vojtechtoman Aug 15, 2019

Contributor

Minor: do we have a test for an unrecognized root node, such as:

foo:
    map:
        ...

Previously, this would result in an InvalidConfigurationException but it works fine now (the contents of foo are effectively ignored and you end up with an empty config).

This comment has been minimized.

Copy link
@blazember

blazember Aug 15, 2019

Author Contributor

We don't have a test covering this. Essentially, it is the same case as creating the following config

hazelcast:
  foo:
    map:
      ...

, which was also ignored before and is now.

If an unexpected top-level node is found, we can either (1) throw an InvalidConfigurationException saying we found an unexpected configuration node, (2) log the same at warning level, or (3) just ignore it. We can do/ensure (1) and (2) this consistently only at the top-level.

YAML by design is prone to different types of misconfigurations, like wrong indentations in the worst case may leave parts of the config silently unprocessed.

Unless we take (1) - which I think a bit restrictive -, I don't see this as a special case that needs a specific test case. I don't have a strong opinion on this though.

@blazember blazember merged commit d8070ff into hazelcast:master Aug 15, 2019
1 check passed
1 check passed
default Test PASSed.
Details
@blazember blazember deleted the blazember:4.0/yaml/minor-improvements branch Aug 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.