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

Config replacers scope clash with config import #15810

Conversation

@alex-dukhno
Copy link
Contributor

alex-dukhno commented Oct 17, 2019

This PR contains fix for config replacers and config imports for xml based configuration files (as reported in #15452)

@blazember, could you please take a look? (cc @neilstevenson)

@devOpsHazelcast

This comment has been minimized.

Copy link
Contributor

devOpsHazelcast commented Oct 17, 2019

Can one of the admins verify this patch?

@mmedenjak mmedenjak requested a review from blazember Oct 18, 2019
@mmedenjak mmedenjak added this to the 4.0 milestone Oct 18, 2019
@alex-dukhno alex-dukhno force-pushed the alex-dukhno:config-replacers-scope-clash-with-config-import branch from f91d97b to 6f127b3 Oct 18, 2019
Copy link
Contributor

blazember left a comment

Looks good to me, with pretty nice cleanups in the tests. Left a few minor comments only.

@alex-dukhno Could you please make the changes done to XmlConfigImportVariableReplacementTest to XmlClientConfigImportVariableReplacementTest as well to have a full coverage?

}

private void replaceImportElementsWithActualFileContents(Node root) throws Exception {
private void replaceVariables(Node root) throws Exception {

This comment has been minimized.

Copy link
@blazember

blazember Nov 4, 2019

Contributor

This and traverseChildrenAndReplaceVariables(root) seem to be duplicate methods. Could this be a leftover?

This comment has been minimized.

Copy link
@blazember

blazember Nov 15, 2019

Contributor

Now traverseChildrenAndReplaceVariables is an unused method. Please don't forget to remove it in #15824.

createFileWithContent("hz1", "xml", ""),
createFileWithContent("hz2", "xml", ""),
createFileWithContent("hz3", "xml", "")
);

This comment has been minimized.

Copy link
@blazember

blazember Nov 4, 2019

Contributor

Nice cleanup 👍

return this;
}

public MapXmlStoreConfigBuilder withInitialMode(String initialMode) {

This comment has been minimized.

Copy link
@blazember

blazember Nov 4, 2019

Contributor

You can use the MapStoreConfig.InitialLoadMode enum here.

@alex-dukhno alex-dukhno requested a review from hazelcast/clients as a code owner Nov 4, 2019
@alex-dukhno alex-dukhno force-pushed the alex-dukhno:config-replacers-scope-clash-with-config-import branch from 9a5ac2e to 9ae11da Nov 4, 2019
@alex-dukhno

This comment has been minimized.

Copy link
Contributor Author

alex-dukhno commented Nov 4, 2019

@blazember I added tests to XmlClientConfigImportVariableReplacementTest, did some cleanup (I'll make some more in #15824) and rebased over latest master.

Thank you for your review.

@blazember

This comment has been minimized.

Copy link
Contributor

blazember commented Nov 4, 2019

run-lab-run

import java.io.IOException;
import java.util.function.Function;

public class IOUtils {

This comment has been minimized.

Copy link
@blazember

blazember Nov 4, 2019

Contributor

This helper feels to be specific to config, I think it should be under com.hazelcast.config.helpers. We have DeclarativeConfigFileHelper there for similar reasons. It doesn't use static methods, but I think these methods can go there. Could you please check if we can add them there?

@blazember

This comment has been minimized.

Copy link
Contributor

blazember commented Nov 4, 2019

@alex-dukhno Thank you for your quick response. I added one more comment for consideration. Also, there are a few checkstyle errors, you can check them with

mvn clean install -DskipTests -Pcheckstyle,spotbugs

in the project root. Could you please take a look?

@blazember

This comment has been minimized.

Copy link
Contributor

blazember commented Nov 11, 2019

run-lab-run

Copy link
Contributor

blazember left a comment

Thanks for the fix @alex-dukhno! It looks good to me.

}

private void replaceImportElementsWithActualFileContents(Node root) throws Exception {
private void replaceVariables(Node root) throws Exception {

This comment has been minimized.

Copy link
@blazember

blazember Nov 15, 2019

Contributor

Now traverseChildrenAndReplaceVariables is an unused method. Please don't forget to remove it in #15824.

@sancar
sancar approved these changes Nov 18, 2019
@blazember blazember merged commit 8bea2b7 into hazelcast:master Nov 18, 2019
1 check passed
1 check passed
default Test PASSed.
Details
@blazember

This comment has been minimized.

Copy link
Contributor

blazember commented Nov 18, 2019

Thank you for the fix @alex-dukhno, a really nice PR 👍

@alex-dukhno alex-dukhno deleted the alex-dukhno:config-replacers-scope-clash-with-config-import branch Nov 18, 2019
petrpleshachkov pushed a commit to petrpleshachkov/hazelcast that referenced this pull request Nov 22, 2019
This change contains fix for config replacers and config imports for xml based configuration files (as reported in hazelcast#15452)
blazember added a commit that referenced this pull request Nov 25, 2019
Consequent PR for #15810 to fix scope clash between config replacers and imports in yaml files.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.