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

fluent-bit-plugin: re-enable failing JSON marshaller tests; pass error instead of logging and ignoring #1455

Merged
merged 1 commit into from
Jan 6, 2020

Conversation

JensErat
Copy link
Contributor

What this PR does / why we need it:

fluent-bit-plugin: re-enable JSON marshal tests

My upstream bugfix to json-iterator/go was accepted. This commit
updates to the newest upstream release, adds checks for the returned
error message and reenables the commented-out JSON marshaller tests.

fluent-bit-plugin: pass error message

Re-enabling the JSON marshaller tests that should return an error
message, I realized sendMessage logged and ignored marshalling errors
instead of passing them to the caller.

Which issue(s) this PR fixes:

Follow-up to PR #1310.

Special notes for your reviewer:

I do not see a reason for logging and ignoring the error here, probably just a left-over from cleanign up the initial code that did lots of this. One could argue that at least the other lines get shipped with this behavior, but then again other parts of the function already return errors instead of logging also for single-log-message-errors.

Checklist

  • Documentation added
  • Tests updated

My upstream bugfix to `json-iterator/go` was accepted. This commit
updates to the newest upstream release, adds checks for the returned
error message and reenables the commented-out JSON marshaller tests.

Re-enabling the JSON marshaller tests that should return an error
message, I realized `sendMessage` logged and ignored marshalling errors
instead of passing them to the caller.

Signed-off-by: Jens Erat <email@jenserat.de>
Copy link
Contributor

@cyriltovena cyriltovena left a comment

Choose a reason for hiding this comment

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

LGTM

@cyriltovena cyriltovena merged commit c9016f4 into grafana:master Jan 6, 2020
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.

None yet

2 participants