-
Notifications
You must be signed in to change notification settings - Fork 80
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
use EULA GeoIP2 Database with auto-update in Elastic license #176
use EULA GeoIP2 Database with auto-update in Elastic license #176
Conversation
|
||
def load_database_manager? | ||
begin | ||
require_relative "#{LogStash::Environment::LOGSTASH_HOME}/x-pack/lib/filters/geoip/database_manager" |
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.
try to load the database manager from xpack
require_relative "#{LogStash::Environment::LOGSTASH_HOME}/x-pack/lib/filters/geoip/database_manager" | ||
true | ||
rescue LoadError => e | ||
@logger.info("DatabaseManager is not in classpath", :version => LOGSTASH_VERSION, :exception => e) |
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.
log in info
level for old version user
This PR is pending on new docker image logstash-8.0.0-SNAPSHOT |
1c65082
to
4f8b64c
Compare
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.
@kaisecheng I'left a couple of comments, one minor method extraction and a doubt about versioned tests.
lib/logstash/filters/geoip.rb
Outdated
raise "You must specify 'database => ...' in your geoip filter (I looked for '#{@database}')" | ||
end | ||
end | ||
database_path = if load_database_manager? |
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 would extract this if in a separate method, for example select_database_path
so the that it could also be separately tested and at the same time streamlines the actions of the register
expect(::File.exist?(METADATA_PATH)).to be_falsey | ||
end | ||
end | ||
end if MAJOR < 7 || (MAJOR == 7 && MINOR <= 12) |
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've a doubt on this conditional tests. They decision of which is executed is based on the values present in https://github.com/logstash-plugins/.ci/blob/b20255c2da0c7fa82e9a247f6366e2da61b47e6c/travis/matrix.yml#L18.
Should we grant runs of these tests with Logstash 7.12
and Logstash 7.13+
? In this way we grant no regressions in future releases of the plugin till the End-of-Life for 7.12
, then we can drop those.
WDYT?
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.
As discussed, it is better to pin test against 7.11.0 to prevent losing track on < 7.12 in the future
53ea489
to
4a106a1
Compare
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
This PR is a split from #172
Additionally, the plugin resolves xpack database manager to run auto-update, otherwise, run the offline CC database