-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
GeoIP database add license files #12756
Conversation
end | ||
|
||
# Make sure the path has usable database | ||
def assert_database!(database_path) | ||
raise "failed to load database #{database_path}" unless org.logstash.filters.GeoIPFilter.database_valid?(database_path) | ||
raise "failed to load database #{database_path}" unless org.logstash.filters.geoip.GeoIPFilter.database_valid?(database_path) |
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.
logstash-filter-geoip
rename the package to org.logstash.filters.geoip
and is waiting to merge that cause the failure of CI. Once the plugin is published, CI should be green
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.
Actual code to copy license etc. looks good.
I have a couple cleanup comments related mostly to already-merged code that accidentally pollutes Object
in specs, and a suggestion to bring the new methods introduced into Util
over into the class that uses them because Util
doesn't have clear boundaries.
end | ||
end | ||
|
||
context "assert database" do | ||
xcontext "assert database" do |
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.
why are we skipping this group of assertions?
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.
Because we move theGeoIPFilter.java
from org.logstash.filters
to org.logstash.filters.geoip
in ECS support PR.
Once the plugin release a new version, this test case will reopen again
@@ -6,7 +6,7 @@ | |||
require "digest" | |||
|
|||
def get_vendor_path | |||
::File.expand_path("./vendor/", ::File.dirname(__FILE__)) |
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.
These helpers should be defined in a self-contained module, and included into the relevant specs. Otherwise we pollute Object
(and therefore all objects):
irb(main):001:0> def test; :oh_no; end;
irb(main):002:0* Object.new.test
=> :oh_no
irb(main):003:0> nil.test
=> :oh_no
irb(main):004:0> true.test
=> :oh_no
irb(main):005:0> 1.test
=> :oh_no
irb(main):006:0> "hello".test
=> :oh_no
The risk here is that we create a situation where our helpers in specs can accidentally provide the methods that they are attempting to specify, leaving the actual methods untested.
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.
😭 :oh_no
it "write the current time" do | ||
dbm.save_timestamp(DEFAULT_CITY_DB_PATH) | ||
|
||
metadata = dbm.get_metadata.last | ||
expect(metadata[DatabaseMetadata::Column::DATABASE_TYPE]).to eq("City") | ||
past = metadata[DatabaseMetadata::Column::UPDATE_AT] | ||
expect(Time.now.to_i - past.to_i).to be < 100 | ||
expect(metadata[DatabaseMetadata::Column::GZ_MD5]).to eq('') | ||
expect(metadata[DatabaseMetadata::Column::GZ_MD5]).not_to be_empty | ||
expect(metadata[DatabaseMetadata::Column::GZ_MD5]).to eq(md5(DEFAULT_CITY_GZ_PATH)) |
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'm having a hard time finding where this EDIT: we are in fact polluting md5
method is coming from, and am worried that we are polluting the Object
space somehow in these specs in a way that is not replicated in released artifacts.Object
, but not in the way I had guessed. See other comment about our spec helpers.
I see that this spec opens up the Geoip filter's definition, then opens up a spec context with describe
, effectively monkey-patching the filter definition in the process. This is at best unclear, and at worst will cause us to validate behaviour provided by the specs themselves instead of code available from lib.
module LogStash module Filters module Geoip
describe DatabaseMetadata, :aggregate_failures do
# ...
end
end end end
Should be:
context LogStash::Filters::Geoip do
describe DatabaseMetadata, :aggregate_failures do
# ...
end
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.
LGTM 👍🏼
Let's get the dependent plugin released and get these specs to green before merging.
Once that plugin is released, you will likely need to bump its version here either manually or with the tools/release/bump_plugin_versions.rb
helper.
There may be a related follow-up task of pinning the version of the GeoIP filter that introduces the package hierarchy change in logstash-core.gemspec
, or at least doing a I think that adding it as a development dependency makes sense here and would probably allow a user to uninstall the plugin if they so choose. But I haven't tested the theory and could be wrong.
diff --git a/logstash-core/logstash-core.gemspec b/logstash-core/logstash-core.gemspec
index 03a5cbedc..257aaf1f2 100644
--- a/logstash-core/logstash-core.gemspec
+++ b/logstash-core/logstash-core.gemspec
@@ -74,6 +74,7 @@ Gem::Specification.new do |gem|
gem.add_runtime_dependency "manticore", '~> 0.6'
# xpack geoip database service
+ gem.add_development_dependency 'logstash-filter-geoip', '~> 7.1'
gem.add_dependency 'faraday' #(MIT license)
gem.add_dependency 'down', '~> 5.2.0' #(MIT license)
gem.add_dependency 'tzinfo-data' #(MIT license)
add test dependency
GeoIP database service provides LICENSE.txt and COPYRIGHT.txt along with the database in .tgz Fixed: elastic#12560
As the legal team request, Logstash should provide
LICENSE.txt
andCOPYRIGHT.txt
along with the databaseInfra team bundles the license files with the database in tar gzip. The download change from .gz to .tgz
The license files are decompressed alongside the database.
CI will be green once the plugin is published