Remove the Arcsight module and the modules framework#16794
Remove the Arcsight module and the modules framework#16794kaisecheng merged 7 commits intoelastic:mainfrom
Conversation
| "xpack.management.elasticsearch.ssl.cipher_suites", | ||
| "xpack.geoip.download.endpoint", | ||
| "xpack.geoip.downloader.enabled", | ||
| "cloud.id", |
There was a problem hiding this comment.
Are these "cloud" settings related to the module being removed?
There was a problem hiding this comment.
I see these are removed in i18n etc too.
There was a problem hiding this comment.
Is cloud.id here only relevant to modules, or is this where it is also set for central management and internal collection?
There was a problem hiding this comment.
cloud.id is only used for modules
CPM uses xpack.management.elasticsearch.cloud_id. Legacy monitoring uses xpack.monitoring.elasticsearch.cloud_id
donoghuc
left a comment
donoghuc
left a comment
There was a problem hiding this comment.
Looks like a clean removal. +28 −7,374 is pretty nice!
robbavey
left a comment
robbavey
left a comment
There was a problem hiding this comment.
Some questions about cloud.id
| # var.PLUGIN_TYPE.PLUGIN_NAME.KEY | ||
| # | ||
| # modules: | ||
| # |
There was a problem hiding this comment.
The Cloud Settings below still refer to module specific settings var.elasticsearch.hosts and var.kibana.host, etc.
If these settings are not relevant at all, we should remove them. If they are relevant to central management and internal collection, we should move and rework them
| "xpack.management.elasticsearch.ssl.cipher_suites", | ||
| "xpack.geoip.download.endpoint", | ||
| "xpack.geoip.downloader.enabled", | ||
| "cloud.id", |
There was a problem hiding this comment.
Is cloud.id here only relevant to modules, or is this where it is also set for central management and internal collection?
|
I think there are some additional removals - I don't believe we use this ElasticsearchClient outside of Modules code, despite it living outside of the main modules code area... |
|
💚 Build Succeeded
History
|
|
@robbavey this is ready for a second look |
robbavey
left a comment
robbavey
left a comment
There was a problem hiding this comment.
This is so awesome! I do have a couple of questions before approval:
- Do we know why those two
requirestatements suddenly needed to be added? Was it related to this PR, or a general issue? - It didn't look like it from reading through the code, but did we confirm whether or not there are dependencies that we import that we no longer need to after this PR?
| require "spec_helper" | ||
| require "open-uri" | ||
| require "webmock/rspec" | ||
| require "faraday" |
There was a problem hiding this comment.
Out of curiosity, do you know what changed that forced this change? Is it part of this change? Or something outside of it?
There was a problem hiding this comment.
This is caused by the removal of elasticsearch_client.rb and the import in runner_spec.rb. The elasticsearch related classes somehow include Faraday.
The import in other spec files affect the global namespace for the entire test suite. You can add require "faraday" in runner_spec and get a green run of ./gradlew :logstash-core:rubyTests, but it will fail when running a single rspec file as webserver_spec itself doesn't link faraday.
|
Hi, We are upgrading to logstash v9.0.1 and there is issue observed in unit-test in pipeline. Thanks & regards, |
5 participants
Release notes
Remove Arcsight module
What does this PR do?
Remove all module related code
The removal of doc is addressed in #16796
Why is it important/What is the impact to the user?
All modules are removed in v9.
Checklist
Author's Checklist
bin/logstash --modules arcsight --setupgivesERROR: Unrecognised option '--modules'Your settings are invalid. Reason: Setting "modules" doesn't existHow to test this PR locally
Related issues
Use cases
Screenshots
Logs