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

Feat: added target => namespace support + ECS compatibility #7

Merged
merged 10 commits into from
Jul 28, 2021

Conversation

kares
Copy link
Contributor

@kares kares commented Mar 27, 2020

We're introducing a target => ... configuration option for the codec, to aid ECS support.
When target isn't set in ECS mode, we do the usual info log message.

@kares kares changed the title Feat: (optional) event target => namespace support Feat: ECS compatibility (target support) Jul 14, 2021
@kares kares marked this pull request as ready for review July 14, 2021 19:13
@kares kares changed the title Feat: ECS compatibility (target support) Feat: added target => namespace support + ECS compatibility Jul 14, 2021
@kares kares requested a review from karenzone July 14, 2021 19:35
@andsel andsel self-requested a review July 15, 2021 16:12
Copy link

@andsel andsel left a comment

Choose a reason for hiding this comment

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

LGTM

left just a couple of nitpicks, that you can skip

@@ -1,5 +1,9 @@
## 1.1.0
- Feat: added target => namespace support + ECS compatibility [#7](https://github.com/logstash-plugins/logstash-codec-csv/pull/7)
Copy link

Choose a reason for hiding this comment

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

Suggested change
- Feat: added target => namespace support + ECS compatibility [#7](https://github.com/logstash-plugins/logstash-codec-csv/pull/7)
- Added `target` configuration option to namespace the read CSV field and added ECS compatibility [#7](https://github.com/logstash-plugins/logstash-codec-csv/pull/7)


it "extract all the values" do
it "return an event from CSV data" do
event_count = 0
Copy link

Choose a reason for hiding this comment

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

event_count is used as an execution guard, sop maybe a boolean could expose better the intention to verify the codec execution.

event_decoded = false
codec.decode(data) do |event|
  event_decoded = true
  ...
end  
expect( event_decoded ).to be true

Copy link
Contributor

@karenzone karenzone left a comment

Choose a reason for hiding this comment

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

Builds cleanly and LGTM

@kares kares merged commit ce24560 into logstash-plugins:master Jul 28, 2021
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.

3 participants