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

Add unittest and documentations for BDK configuration #176

Merged
merged 3 commits into from
Aug 6, 2020

Conversation

symphony-hong
Copy link
Contributor

Add unittest and documentations for BDK configuration

Move BdkConfigException to com.symphony.bdk.core.config.exceptions
to avoid many types of exceptions in a single packages

Add a condition in BdkConfigParser#parse to check if invalid YAML config is parsed to a single
text node.

Verified

This commit was signed with the committer’s verified signature.
Move BdkConfigException to com.symphony.bdk.core.config.exceptions
to avoid many types of exceptions in a single packages

Add a condition in BdkConfigParser#parse to check if invalid YAML config is parsed to a single
text node.
@symphony-hong symphony-hong requested review from thibauult and a team August 6, 2020 09:49
@@ -35,7 +35,9 @@ public static JsonNode parse(InputStream inputStream) throws BdkConfigException
}

try {
return YAML_MAPPER.readTree(content);
JsonNode jsonNode = YAML_MAPPER.readTree(content);
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably make BdkConfigParser internal right?
I see it is already does not have public visibility, maybe annotate it with API guardian?

Verified

This commit was signed with the committer’s verified signature.
@@ -0,0 +1,46 @@
#BDK Configuration
Copy link
Member

Choose a reason for hiding this comment

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

Just # Configuration

public void loadFromYamlFileTest() throws JsonProcessingException, BdkConfigException {
String configPath = System.getProperty("user.dir") + "/src/test/resources/config.yaml";
public void loadFromYamlFileTest() throws BdkConfigException {
String configPath = System.getProperty("user.dir") + "/src/test/resources/config/config.yaml";
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure that this is the safest way to locate the test resources. I'd personally use another strategy :

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.

It's a good idea! I will try that way. 👍

Comment on lines 68 to 69
assertEquals(config.getBot().getPrivateKeyPath(), "/Users/thibault.pensec/local/conf/agent/privatekey.pem");
assertEquals(config.getApp().getPrivateKeyPath(), "/Users/thibault.pensec/local/conf/agent/privatekey.pem");
Copy link
Member

Choose a reason for hiding this comment

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

Please remove "thibault.pensec" from the test data 😅 I'll do the same with my PR !

"agentHost": "devx1.symphony.com",
"keyAuthHost": "devx1.symphony.com",
"botUsername": "tibot",
"botPrivateKeyPath": "/Users/thibault.pensec/local/conf/agent/",
Copy link
Member

Choose a reason for hiding this comment

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

Please remove "thibault.pensec" !

"botPrivateKeyPath": "/Users/thibault.pensec/local/conf/agent/",
"botPrivateKeyName": "privatekey.pem",
"appId": "tibapp",
"appPrivateKeyPath": "/Users/thibault.pensec/local/conf/agent/",
Copy link
Member

Choose a reason for hiding this comment

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

Same...

Verified

This commit was signed with the committer’s verified signature.
Update documentation for configuration.

Update test resources.
@symphony-hong symphony-hong requested a review from thibauult August 6, 2020 14:55
Copy link
Member

@thibauult thibauult left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@symphony-hong symphony-hong merged commit cc8e0ec into finos:master Aug 6, 2020
@symphony-hong symphony-hong deleted the APP-2884 branch August 19, 2020 16:01
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

3 participants