-
Notifications
You must be signed in to change notification settings - Fork 28
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
Fixed Gemfile to permit development with local Logstash clone #82
Conversation
Gemfile
Outdated
|
||
if Dir.exist?(logstash_path) && ENV["LOGSTASH_SOURCE"] == "1" | ||
if Dir.exist?(logstash_path) && use_logstash_source |
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 don't think use_logstash_source is really needed, because of my previous comment.
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.
It's the same like in Beats input: https://github.com/logstash-plugins/logstash-input-beats/blob/main/Gemfile#L8
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.
While it may be, I still don't think it is a necessary change.
Example 1: In this code, you are setting use_logstash_source to 1 if LOGSTASH_SOURCE is equal to 1.
use_logstash_source = ENV['LOGSTASH_SOURCE'] && ENV['LOGSTASH_SOURCE'].to_s == '1'
if Dir.exist?(logstash_path) && use_logstash_source
Example 2: The above is the same as the following code, which is evaluating if LOGSTASH_SOURCE is equal to 1.
if Dir.exist?(logstash_path) && ENV["LOGSTASH_SOURCE"] == "1"
Meaning that the change is not really necessary.
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 you are right,
ENV['LOGSTASH_SOURCE'] && ENV['LOGSTASH_SOURCE'].to_s == '1'
provides the same result as
ENV['LOGSTASH_SOURCE'] == '1'
but my point here is to use the exact Gemfile
that's used in all the other plugins. If we change here should we align also the others?
Gemfile
Outdated
gem 'logstash-core', :path => "#{logstash_path}/logstash-core" | ||
gem 'logstash-core-plugin-api', :path => "#{logstash_path}/logstash-core-plugin-api" | ||
end | ||
|
||
if RUBY_VERSION == "1.9.3" | ||
gem 'rake', '12.2.1' | ||
end | ||
|
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.
There's no reason to add 3 empty lines here.
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.
Approving, but before landing consider my last comment provided that it is not necessary to make the change you are suggesting.
Gemfile
Outdated
|
||
if Dir.exist?(logstash_path) && ENV["LOGSTASH_SOURCE"] == "1" | ||
if Dir.exist?(logstash_path) && use_logstash_source |
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.
While it may be, I still don't think it is a necessary change.
Example 1: In this code, you are setting use_logstash_source to 1 if LOGSTASH_SOURCE is equal to 1.
use_logstash_source = ENV['LOGSTASH_SOURCE'] && ENV['LOGSTASH_SOURCE'].to_s == '1'
if Dir.exist?(logstash_path) && use_logstash_source
Example 2: The above is the same as the following code, which is evaluating if LOGSTASH_SOURCE is equal to 1.
if Dir.exist?(logstash_path) && ENV["LOGSTASH_SOURCE"] == "1"
Meaning that the change is not really necessary.
Release notes
[rn:skip]
What does this PR do?
Fix the
Gemfile
to properly set variables to use local Logstash cloneWhy is it important/What is the impact to the user?
Let the developer to use local clone, when configuring
LOGSTASH_PATH
andLOGSTASH_SOURCE
instead of raisingjava.lang.NoClassDefFoundError
Checklist
[ ] I have commented my code, particularly in hard-to-understand areas[ ] I have made corresponding changes to the documentation[ ] I have made corresponding change to the default configuration files (and/or docker env variables)[ ] I have added tests that prove my fix is effective or that my feature worksAuthor's Checklist
How to test this PR locally
Follow instruction in #81
Related issues
Use cases
Screenshots
Logs