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

avoiding overwriting of tags with key named as 'tags' #10

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

hselvara
Copy link

Thanks for contributing to Logstash! If you haven't already signed our CLA, here's a handy link: https://www.elastic.co/contributor-agreement/

@hselvara
Copy link
Author

I have signed the CLA and i'm the member of elasticsearch. I have created the pull request for avoiding overwriting of tags with key named as 'tags' #10 . Kindly review the fix.

@jsvd
Copy link
Member

jsvd commented Apr 10, 2017

hey @hselvara I'm not sure what happened during the creation of the PR, but your commit creates a new file instead of modifying lib/logstash/codecs/fluent.rb

@hselvara
Copy link
Author

Hi @jsvd i have updated the PR. Please review.

Copy link
Member

@jsvd jsvd left a comment

Choose a reason for hiding this comment

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

few code style changes. also, can you add a test that shows what this feature allows? thank you

@@ -40,7 +40,8 @@ def decode(data, &block)
end # def decode

def encode(event)
tag = event.get("tags") || "log"
tag = event.get("tags") || "log"
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 this indentation changes, this will make the PR less noisy and more concise.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah i completed that. Thanks for notifying

@@ -70,31 +71,34 @@ def decode_event(data, &block)
entries_decoder.feed_each(entries) do |entry|
epochtime = entry[0]
map = entry[1]
event = LogStash::Event.new(map.merge(
arr= []
Copy link
Member

Choose a reason for hiding this comment

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

please try to follow the core conventions present in this file. when assigning to a variable, we use spaces before and after the equals sign, so arr = [] instead of arr= []

Copy link
Author

Choose a reason for hiding this comment

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

I mad the changes. Thanks for Notifying

@hselvara
Copy link
Author

Hi Jsvd,

I have updated your comments. Please review

@jsvd
Copy link
Member

jsvd commented Jun 30, 2017

@hselvara can you rebase, please? the PR now rewrites about 16 files for no apparent reason

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.

2 participants