-
Notifications
You must be signed in to change notification settings - Fork 97
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
Upgrade Fluentd to v1.2.5 and bump gem version to v0.7.0. #240
Conversation
Pending for soak test result. |
e769283
to
c88db9e
Compare
@@ -19,7 +19,7 @@ eos | |||
gem.test_files = gem.files.grep(/^(test)/) | |||
gem.require_paths = ['lib'] | |||
|
|||
gem.add_runtime_dependency 'fluentd', '~> 0.10' | |||
gem.add_runtime_dependency 'fluentd', '~> 1.2', '>= 1.2.5' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternative
gem.add_runtime_dependency 'fluentd', '>= 0.10', '< 2'
If we'd like to have some backward compatibility for users who have Fluentd v1.2 incompatible configuration. But that means they could be exposed to the memory issue.`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's fine. We'd stop supporting earlier versions of fluentd after the version bump.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool
Soak test passed for Fluentd v1.2.5.rc1. Kicking off this PR so that we can build a full package. Will run soak test against the new package again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments.
@@ -19,7 +19,7 @@ eos | |||
gem.test_files = gem.files.grep(/^(test)/) | |||
gem.require_paths = ['lib'] | |||
|
|||
gem.add_runtime_dependency 'fluentd', '~> 0.10' | |||
gem.add_runtime_dependency 'fluentd', '~> 1.2', '>= 1.2.5' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's fine. We'd stop supporting earlier versions of fluentd after the version bump.
@@ -228,7 +228,7 @@ module InternalConstants | |||
Fluent::Plugin.register_output('google_cloud', self) | |||
|
|||
PLUGIN_NAME = 'Fluentd Google Cloud Logging plugin'.freeze | |||
PLUGIN_VERSION = '0.6.23'.freeze | |||
PLUGIN_VERSION = '0.7.0'.freeze |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #245.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. I will rebase when that's merged.
fluent-plugin-google-cloud.gemspec
Outdated
@@ -10,7 +10,7 @@ eos | |||
gem.homepage = | |||
'https://github.com/GoogleCloudPlatform/fluent-plugin-google-cloud' | |||
gem.license = 'Apache-2.0' | |||
gem.version = '0.6.23' | |||
gem.version = '0.7.0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a bunch of pending changes. Do we want to wait until those are in before bumping the version?
Alternatively, should we bump the version off a clean release (so that pulling in fluentd 1.2 is the only change)? This would mean releasing 0.6.24 first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that makes perfect sense. I think we are releasing at v0.6.24
next week. This will definitely wait till after that.
c88db9e
to
1e1a9b2
Compare
Rebased off master after #245 was merged to master. PTAL |
1902e11
to
299181f
Compare
Rebased off master again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
299181f
to
d5b9bf8
Compare
d5b9bf8
to
595560b
Compare
…)" This reverts commit 906df5e.
Upgrade Fluentd to v1.2.5 (latest as of Aug 24, 2018) to apply this fix fluent/fluentd#1941.
Fixes #251.