Remove the Arcsight module and the modules framework #16794
Conversation
| @@ -126,8 +126,6 @@ var validSettings = []string{ | |||
| "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
| # 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
| @@ -126,8 +126,6 @@ var validSettings = []string{ | |||
| "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 |
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?
| @@ -21,6 +21,8 @@ | |||
| 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.
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