Added Logstash OpenSearch output plugin mapping to converter #756
Conversation
| final String logstashAttributeName = logstashAttribute.getAttributeName(); | ||
|
|
||
| if (mappedAttributeNames.containsKey(logstashAttributeName)) { | ||
| if (mappedAttributeNames.get(logstashAttributeName).startsWith("!")) { |
There was a problem hiding this comment.
I think you should extract this into a variable:
String attributeValue = mappedAttributeNames.get(logstashAttributeName)
It is used three times, so consolidating it into a variable will aid readability.
| @Test | ||
| void mapAttributes_with_negation_expression_negates_boolean_value() { | ||
| final String dataPrepperAttribute = "!".concat(UUID.randomUUID().toString()); | ||
| boolean value = true; |
There was a problem hiding this comment.
For boolean tests, we should use parameterized tests. It helps ensure that the code is not written to favor one boolean value over another.
@ParameterizedTest
@ValuesSource(booleans = { true, false } )
void mapAttributes_with_negation_expression_negates_boolean_value(boolean inputValue)
I used inputValue instead of value for clarity, and recommend a similar change as well.
| @@ -0,0 +1,7 @@ | |||
| pluginName: opensearch | |||
There was a problem hiding this comment.
Please add an integration test case for this.
See the LogstashConfigConverterIT Javadocs for how to add a new case. It only requires creating input and expected files.
| if (mappedAttributeNames.get(logstashAttributeName).startsWith("!")) { | ||
| pluginSettings.put( | ||
| mappedAttributeNames.get(logstashAttributeName).substring(1), | ||
| !(Boolean) logstashAttribute.getAttributeValue().getValue() |
There was a problem hiding this comment.
It might be a bit cleaner to verify that the attribute is actually a boolean before casting. So something like
if (mappedAttributeNames.get(logstashAttributeName).startsWith("!") && logstashAttribute.getAttributeValue().getValue() instanceOf Boolean)
There was a problem hiding this comment.
Good call. I also think that a new unit test case would be useful here.
| @@ -114,4 +115,20 @@ void mapAttributes_sets_additional_attributes_to_those_values() { | |||
| assertThat(actualPluginSettings, hasKey(additionalAttributeName)); | |||
| assertThat(actualPluginSettings.get(additionalAttributeName), equalTo(additionalAttributeValue)); | |||
| } | |||
|
|
|||
| @Test | |||
There was a problem hiding this comment.
I would like to see an integration test added that contains the opensearch plugin.
Signed-off-by: Asif Sohail Mohammed <[email protected]>
asifsmohammed
force-pushed
the
opensearch
branch
from
d953d45 to
0eb3f04
Compare
| username: "myuser" | ||
| password: "mypassword" | ||
| index: "my-index" | ||
| insecure: false |
There was a problem hiding this comment.
Nit: I would rather see an integration test with a non-default value for insecure, as it feels like there is some chance that it isn't actually taking into consideration and negating the value from ssl_certificate_verification
There was a problem hiding this comment.
Having two test cases here makes sense.
By the way, default values shouldn't end up in the generated YAML. So there is a good indication that this is working. But, I'm still for adding another input file.
There was a problem hiding this comment.
Thanks for the feedback. Yeah, as David mentioned default values won't be there in YAML files.
I'll add one more integration test.
Signed-off-by: Asif Sohail Mohammed <[email protected]>
asifsmohammed
force-pushed
the
opensearch
branch
from
0eb3f04 to
84eaa84
Compare
...java/org/opensearch/dataprepper/logstash/mapping/AbstractLogstashPluginAttributesMapper.java
Show resolved
Hide resolved
Signed-off-by: Asif Sohail Mohammed [email protected]
Description
Issues Resolved
Resolves #710
Resolves #746
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.