Skip to content
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

separate config options into elasticsearch specific and shared #973

Merged

Conversation

colinsurprenant
Copy link
Contributor

@colinsurprenant colinsurprenant commented Oct 20, 2020

This is work toward Data Streams integration elastic/logstash#12178

Specific elasticsearch option are now into the main elasticsearch class file and shared option have been moved into the PluginMixins::ElasticSearch::APIConfigs namespace.

This is essentially a no-op PR with only the config options reorganization.

I included the removal of both DEFAULT_INDEX_NAME and DEFAULT_ROLLOVER_ALIAS constants which were unused.

@colinsurprenant
Copy link
Contributor Author

@yaauie Agree with your comments. LMKWYT with the modifications.

@colinsurprenant
Copy link
Contributor Author

Bump /cc @jsvd

Copy link
Contributor

@yaauie yaauie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍🏼

Copy link
Member

@jsvd jsvd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a couple of notes, otherwise LGTM

For reference, here's the list of settings that are shared:

❯ grep "config :" lib/logstash/outputs/elasticsearch/api_configs.rb | sort | cut -d " " -f 8-
:api_key, :validate => :password
:bulk_path, :validate => :string
:cacert, :validate => :path
:cloud_auth, :validate => :password
:cloud_id, :validate => :string
:custom_headers, :validate => :hash, :default => {}
:failure_type_logging_whitelist, :validate => :array, :default => []
:healthcheck_path, :validate => :string
:hosts, :validate => :uri, :default => [ DEFAULT_HOST ], :list => true
:http_compression, :validate => :boolean, :default => false
:keystore, :validate => :path
:keystore_password, :validate => :password
:parameters, :validate => :hash
:password, :validate => :password
:path, :validate => :string
:pool_max, :validate => :number, :default => 1000
:pool_max_per_route, :validate => :number, :default => 100
:proxy, :validate => :uri # but empty string is allowed
:resurrect_delay, :validate => :number, :default => 5
:retry_initial_interval, :validate => :number, :default => 2
:retry_max_interval, :validate => :number, :default => 64
:sniffing, :validate => :boolean, :default => false
:sniffing_delay, :validate => :number, :default => 5
:sniffing_path, :validate => :string
:ssl, :validate => :boolean
:ssl_certificate_verification, :validate => :boolean, :default => true
:timeout, :validate => :number, :default => 60
:truststore, :validate => :path
:truststore_password, :validate => :password
:user, :validate => :string
:validate_after_inactivity, :validate => :number, :default => 10000

And the list of configs we'll not use in the datastreams output:

❯ grep "config :" lib/logstash/outputs/elasticsearch.rb | sort | cut -d " " -f 4-
:action, :validate => :string, :default => "index"
:doc_as_upsert, :validate => :boolean, :default => false
:document_id, :validate => :string
:document_type,
:ilm_enabled, :validate => [true, false, 'true', 'false', 'auto'], :default => 'auto'
:ilm_pattern, :validate => :string, :default => '{now/d}-000001'
:ilm_policy, :validate => :string, :default => DEFAULT_POLICY
:ilm_rollover_alias, :validate => :string
:index, :validate => :string
:join_field, :validate => :string, :default => nil
:manage_template, :validate => :boolean, :default => true
:parent, :validate => :string, :default => nil
:pipeline, :validate => :string, :default => nil
:retry_on_conflict, :validate => :number, :default => 1
:routing, :validate => :string
:script, :validate => :string, :default => ""
:script_lang, :validate => :string, :default => "painless"
:script_type, :validate => ["inline", 'indexed', "file"], :default => ["inline"]
:script_var_name, :validate => :string, :default => "event"
:scripted_upsert, :validate => :boolean, :default => false
:template, :validate => :path
:template_name, :validate => :string
:template_overwrite, :validate => :boolean, :default => false
:upsert, :validate => :string, :default => ""
:version, :validate => :string
:version_type, :validate => ["internal", 'external', "external_gt", "external_gte", "force"]

@colinsurprenant
Copy link
Contributor Author

colinsurprenant commented Nov 20, 2020

@yaauie per discussion in #976 I moved the APIConfig into the PluginMixins namespace. Could you please double check that last commit to make sure all is good per our discussion. When good, I'll rebase #976 against this and move the commons code in the mixin.

specific elasticsearch option are now into the main elasticsearch
class file and shared option have been moved into the
PluginMixins::ElasticSearch::APIConfigs namespace.
This is code refactorting that has no end-user impact.
@colinsurprenant colinsurprenant merged commit 889a16e into logstash-plugins:master Dec 1, 2020
@colinsurprenant
Copy link
Contributor Author

Thanks @yaauie @jsvd for the reviews. This is merged and will be part of 10.8.0.

colinsurprenant added a commit that referenced this pull request Dec 2, 2020
This version bump wraps #973 and #976 to refactor methods and configuration options. This release does not contain user-facing changes other than the management/monitoring in logstash core which depends on this plugin should produce less irrelevant licensing logs by benefiting the no-op license checking class.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants