Skip to content

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

Open
hselvara wants to merge 2 commits into
logstash-plugins:mainfrom
hselvara:avoid_overwrite_of_tags
Open

avoiding overwriting of tags with key named as 'tags'#10
hselvara wants to merge 2 commits into
logstash-plugins:mainfrom
hselvara:avoid_overwrite_of_tags

Conversation

@hselvara

Copy link
Copy Markdown

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
Copy Markdown
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

jsvd commented Apr 10, 2017

Copy link
Copy Markdown
Member

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 hselvara force-pushed the avoid_overwrite_of_tags branch from 2fd8df8 to f98ae45 Compare April 13, 2017 12:53
@hselvara

Copy link
Copy Markdown
Author

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

@jsvd jsvd left a comment

Copy link
Copy Markdown
Member

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

Comment thread lib/logstash/codecs/fluent.rb Outdated

def encode(event)
tag = event.get("tags") || "log"
tag = event.get("tags") || "log"

Copy link
Copy Markdown
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
Copy Markdown
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

Comment thread lib/logstash/codecs/fluent.rb Outdated
epochtime = entry[0]
map = entry[1]
event = LogStash::Event.new(map.merge(
arr= []

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Author

Hi Jsvd,

I have updated your comments. Please review

@jsvd

jsvd commented Jun 30, 2017

Copy link
Copy Markdown
Member

@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