Skip to content

Bug 1519213 - NoMethodError if remote_syslog enabled remote_syslog_ta… - #820

Merged
openshift-merge-robot merged 1 commit into
openshift:masterfrom
nhosoi:bz1519213-0
Dec 8, 2017
Merged

Bug 1519213 - NoMethodError if remote_syslog enabled remote_syslog_ta…#820
openshift-merge-robot merged 1 commit into
openshift:masterfrom
nhosoi:bz1519213-0

Conversation

@nhosoi

@nhosoi nhosoi commented Dec 6, 2017

Copy link
Copy Markdown
Contributor

…g_key

fluent-plugin-remote-syslog: replacing tag_key with @tag_key
adding rescue around record[@tag_key]
Note: out_syslog.rb and out_syslog_buffered.rb are copied from
https://github.com/docebo/fluent-plugin-remote-syslog and
locally modified. Once the changes made on the files are
accepted by the plugin, out_syslog.rb and out_syslog_buffered.rb
are to be removed and the changes on Dockerfile* are to be undone.

Adding test cases for tag_key to remote_syslog.sh

@nhosoi
Bug 1519213 - NoMethodError if remote_syslog enabled remote_syslog_ta…
1cea2e1 
…g_key

fluent-plugin-remote-syslog: replacing tag_key with @tag_key
                             adding rescue around record[@tag_key]
Note: out_syslog.rb and out_syslog_buffered.rb are copied from
      https://github.com/docebo/fluent-plugin-remote-syslog and
      locally modified.  Once the changes made on the files are
      accepted by the plugin, out_syslog.rb and out_syslog_buffered.rb
      are to be removed and the changes on Dockerfile* are to be undone.

Adding test cases for tag_key to remote_syslog.sh
@nhosoi
nhosoi requested a review from richm December 6, 2017 22:22
@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 6, 2017
@richm

richm commented Dec 6, 2017

Copy link
Copy Markdown
Contributor

Did you also submit a patch upstream to https://github.com/docebo/fluent-plugin-remote-syslog?

@nhosoi

nhosoi commented Dec 6, 2017

Copy link
Copy Markdown
Contributor Author

Yes, this is the PR.
docebo/fluent-plugin-remote-syslog#12

@nhosoi

nhosoi commented Dec 6, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Not sure what is wrong, but building failed in the upstream fluent-plugin-remote-syslog...

continuous-integration/travis-ci/pr — The Travis CI build could not complete due to an error
https://travis-ci.org/docebo/fluent-plugin-remote-syslog/builds/312670193?utm_source=github_status&utm_medium=notification

The previous PR also failed in the very similar manner...
docebo/fluent-plugin-remote-syslog#11

@nhosoi

nhosoi commented Dec 6, 2017

Copy link
Copy Markdown
Contributor Author

/retest

1 similar comment
@nhosoi

nhosoi commented Dec 7, 2017

Copy link
Copy Markdown
Contributor Author

/retest

richm
richm approved these changes Dec 7, 2017
View reviewed changes

@richm richm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 7, 2017
@richm

richm commented Dec 7, 2017

Copy link
Copy Markdown
Contributor

/cherrypick release-3.7

@openshift-cherrypick-robot

openshift-cherrypick-robot commented Dec 7, 2017

Copy link
Copy Markdown

@richm: once the present PR merges, I will cherry-pick it on top of release-3.7 in a new PR and assign it to you.

Details

In response to this:

/cherrypick release-3.7

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@richm richm added backport/3.7 component/fluentd kind/bug Categorizes issue or PR as related to a bug. release/3.9 labels Dec 8, 2017
@richm

richm commented Dec 8, 2017

Copy link
Copy Markdown
Contributor

why isn't this merging?

@nhosoi

nhosoi commented Dec 8, 2017

Copy link
Copy Markdown
Contributor Author

/retest

@openshift-merge-robot

openshift-merge-robot commented Dec 8, 2017

Copy link
Copy Markdown
Contributor

Automatic merge from submit-queue.

@openshift-merge-robot
openshift-merge-robot merged commit b223dc2 into openshift:master Dec 8, 2017
@openshift-cherrypick-robot

openshift-cherrypick-robot commented Dec 8, 2017

Copy link
Copy Markdown

@richm: new pull request created: #830

Details

In response to this:

/cherrypick release-3.7

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Dec 8, 2017
Merged
openshift-merge-robot added a commit that referenced this pull request Dec 8, 2017
@openshift-merge-robot
Merge pull request #830 from openshift-cherrypick-robot/cherry-pick-8…
56c2e57 
…20-to-release-3.7

Automatic merge from submit-queue.

Automated cherry-pick of #820 on release-3.7

This is an automated cherry-pick of #820

/assign richm
@portante

portante commented Dec 8, 2017

Copy link
Copy Markdown
Contributor

Not sure this is legal to do. We can't just copy source code into our repository like this if it is licensed. I think that code is under the MIT license, at least in the gemspec it says MIT.

@richm

richm commented Dec 8, 2017

Copy link
Copy Markdown
Contributor

I've asked legal for advice

@nhosoi

nhosoi commented Dec 8, 2017

Copy link
Copy Markdown
Contributor Author

@portante, @richm,

I think that code is under the MIT license, at least in the gemspec it says MIT.
Good to hear that.
A community member submitted this pr on 2017-03-22, but there has been no activities in the pr since then.
docebo/fluent-plugin-remote-syslog#9
So, even if the plugin does not have a LICENSE file nor a license section in each source file, the plugin is "protected"?

@portante

portante commented Dec 8, 2017

Copy link
Copy Markdown
Contributor

@nhosoi, better safe than sorry and check with legal.

@nhosoi

nhosoi commented Dec 8, 2017

Copy link
Copy Markdown
Contributor Author

@porante, you are right. I've opened this pr following the suggestion from the Legal team.
#836

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@richm richm richm approved these changes

Assignees

@richm richm

Labels

backport/completed backport/3.7 component/fluentd kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. release/3.9 size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@nhosoi @richm @openshift-cherrypick-robot @openshift-merge-robot @portante @openshift-ci-robot