Added configConverter which converts conf to yaml #584

asifsmohammed · 2021-11-12T21:47:05Z

Description

Added ConfigConverter which takes .conf file path and returns .yaml file path

Issues Resolved

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff

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.

codecov-commenter · 2021-11-12T22:06:45Z

Codecov Report

Merging #584 (4fb2563) into main (42c5954) will increase coverage by 0.17%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main     #584      +/-   ##
============================================
+ Coverage     92.12%   92.29%   +0.17%     
  Complexity      569      569              
============================================
  Files            71       71              
  Lines          1727     1727              
  Branches        144      144              
============================================
+ Hits           1591     1594       +3     
+ Misses          105      102       -3     
  Partials         31       31

Impacted Files	Coverage Δ
...java/com/amazon/dataprepper/pipeline/Pipeline.java	`92.85% <0.00%> (+4.28%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 42c5954...4fb2563. Read the comment docs.

dlvenable · 2021-11-12T22:17:41Z

...on/src/test/java/org/opensearch/dataprepper/logstash/parser/LogstashConfigConverterTest.java

+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+
+class LogstashConfigConverterTest {


This test will have some overlap with the integration tests. If we aren't going to mock the internal classes, I think I'd prefer that we remove this test and only have the integration test to cover it.

I think we can only have an integration test for this.

Then let's just remove this test and the input file.

dlvenable · 2021-11-12T22:19:01Z

...ration/src/main/java/org/opensearch/dataprepper/logstash/parser/LogstashConfigConverter.java

+
+        ObjectMapper mapper = new ObjectMapper(factory);
+
+        String yamlFilePath = outputDirectory + "logstash.yaml";


Using string append to create paths can be problematic. Please use File or Paths to form this string.

dlvenable · 2021-11-12T22:21:06Z

...ration/src/main/java/org/opensearch/dataprepper/logstash/parser/LogstashConfigConverter.java

+
+        LogstashMapper logstashMapper = new LogstashMapper();
+        PipelineModel pipelineModel = logstashMapper.mapPipeline(logstashConfiguration);
+        Map<String, Object> pipeline = Collections.singletonMap("log-pipeline", pipelineModel);


We don't know if the pipeline is a log-pipeline or not. I'd be interested in using the filename as the pipeline name. For example a file named local-log-ingest.yaml might be given the name local-log-ingest.

If we don't take that approach, maybe use a constant value like logstash-converted-pipeline.

dlvenable · 2021-11-12T22:22:10Z

...ration/src/main/java/org/opensearch/dataprepper/logstash/parser/LogstashConfigConverter.java

+
+        ObjectMapper mapper = new ObjectMapper(factory);
+
+        String yamlFilePath = outputDirectory + "logstash.yaml";


Should we use the name of the original file in the generated file?

e.g. my-configuration-file.conf -> my-configuration-file.yaml.

That makes sense because the user knows their file name, so they can just look for .yaml file for their .conf file

dlvenable · 2021-11-12T22:30:56Z

...ration/src/main/java/org/opensearch/dataprepper/logstash/parser/LogstashConfigConverter.java

@@ -0,0 +1,56 @@
+package org.opensearch.dataprepper.logstash.parser;


Also, I think this should be in the org.opensearch.dataprepper.logstash package. You will need to update your usage of LogstashVisitor with the full package name.

Like this:

org.opensearch.dataprepper.logstash.parser.LogstashVisitor visitor = new org.opensearch.dataprepper.logstash.parser.LogstashVisitor();

Signed-off-by: Asif Sohail Mohammed <[email protected]>

dlvenable · 2021-11-12T23:19:00Z

...configuration/src/main/java/org/opensearch/dataprepper/logstash/LogstashConfigConverter.java

+        LogstashParser parser = new LogstashParser(tokens);
+        ParseTree tree = parser.config();
+
+        org.opensearch.dataprepper.logstash.parser.LogstashVisitor visitor = new LogstashVisitor();


You need to have the fully qualified name on the right-side of the assignment, along with the new.

org.opensearch.dataprepper.logstash.parser.LogstashVisitor visitor = new org.opensearch.dataprepper.logstash.parser.LogstashVisitor();

sshivanii · 2021-11-12T23:22:26Z

...configuration/src/main/java/org/opensearch/dataprepper/logstash/LogstashConfigConverter.java

+        factory.disable(YAMLGenerator.Feature.SPLIT_LINES);
+        factory.enable(YAMLGenerator.Feature.LITERAL_BLOCK_STYLE);
+
+        ObjectMapper mapper = new ObjectMapper(factory);


You can simplify this by using the Builder pattern like:

ObjectMapper mapper = new ObjectMapper(YAMLFactory.builder() .disable(YAMLGenerator.Feature.WRITE_DOC_START_MARKER) .disable(YAMLGenerator.Feature.SPLIT_LINES) .enable(YAMLGenerator.Feature.INDENT_ARRAYS_WITH_INDICATOR) .enable(YAMLGenerator.Feature.LITERAL_BLOCK_STYLE) .build());

sshivanii · 2021-11-12T23:28:08Z

...configuration/src/main/java/org/opensearch/dataprepper/logstash/LogstashConfigConverter.java

+                .enable(YAMLGenerator.Feature.LITERAL_BLOCK_STYLE)
+                .build());
+
+        String confFileName = configurationFilePath.getFileName().toString();


Do we have all the fields that should not be changed final?

No, will update it

Signed-off-by: Asif Sohail Mohammed <[email protected]>

chenqi0805

Checked comments from @sshivanii addressed.

asifsmohammed requested a review from a team as a code owner November 12, 2021 21:47

asifsmohammed force-pushed the configconverter branch from c7c8c66 to e032adf Compare November 12, 2021 21:58

asifsmohammed requested review from sshivanii and dlvenable November 12, 2021 22:22

dlvenable requested changes Nov 12, 2021

View reviewed changes

dlvenable reviewed Nov 12, 2021

View reviewed changes

Added configConverter which converts conf to yaml

e594b87

Signed-off-by: Asif Sohail Mohammed <[email protected]>

asifsmohammed force-pushed the configconverter branch from e032adf to 219a8c1 Compare November 12, 2021 23:13

dlvenable reviewed Nov 12, 2021

View reviewed changes

sshivanii reviewed Nov 12, 2021

View reviewed changes

asifsmohammed force-pushed the configconverter branch from 219a8c1 to 7cbd613 Compare November 12, 2021 23:26

sshivanii reviewed Nov 12, 2021

View reviewed changes

Updated configConverter and removed the tests

4fb2563

Signed-off-by: Asif Sohail Mohammed <[email protected]>

asifsmohammed force-pushed the configconverter branch from 7cbd613 to 4fb2563 Compare November 12, 2021 23:36

dlvenable approved these changes Nov 13, 2021

View reviewed changes

chenqi0805 approved these changes Nov 13, 2021

View reviewed changes

asifsmohammed merged commit 6412e9b into opensearch-project:main Nov 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added configConverter which converts conf to yaml #584

Added configConverter which converts conf to yaml #584

asifsmohammed commented Nov 12, 2021 •

edited

Loading

codecov-commenter commented Nov 12, 2021 •

edited

Loading

dlvenable Nov 12, 2021

asifsmohammed Nov 12, 2021

dlvenable Nov 12, 2021

dlvenable Nov 12, 2021

dlvenable Nov 12, 2021

dlvenable Nov 12, 2021

asifsmohammed Nov 12, 2021 •

edited

Loading

dlvenable Nov 12, 2021

dlvenable Nov 12, 2021

sshivanii Nov 12, 2021

sshivanii Nov 12, 2021

asifsmohammed Nov 12, 2021

chenqi0805 left a comment


		ObjectMapper mapper = new ObjectMapper(factory);

		String yamlFilePath = outputDirectory + "logstash.yaml";

		@@ -0,0 +1,56 @@
		package org.opensearch.dataprepper.logstash.parser;

Added configConverter which converts conf to yaml #584

Added configConverter which converts conf to yaml #584

Conversation

asifsmohammed commented Nov 12, 2021 • edited Loading

Description

Issues Resolved

Check List

codecov-commenter commented Nov 12, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asifsmohammed Nov 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenqi0805 left a comment

Choose a reason for hiding this comment

asifsmohammed commented Nov 12, 2021 •

edited

Loading

codecov-commenter commented Nov 12, 2021 •

edited

Loading

asifsmohammed Nov 12, 2021 •

edited

Loading