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.
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.
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.
why are we skipping this group of assertions?
There was a problem hiding this comment.
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.
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_noThe 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.
| 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.
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 endShould be:
context LogStash::Filters::Geoip do
describe DatabaseMetadata, :aggregate_failures do
# ...
end
end
There was a problem hiding this comment.
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.txtandCOPYRIGHT.txtalong 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