Skip to content

Add xpack.eql.enabled feature flag, disabled by default. Enabled for gradle run task and integration tests. #51370

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

Merged
merged 3 commits into from
Jan 24, 2020

Conversation

aleksmaus
Copy link
Contributor

Related to #49581

@aleksmaus aleksmaus force-pushed the feature/eql_feature_flag branch from faca844 to 68f09e3 Compare January 23, 2020 19:10
Copy link
Contributor

@imotov imotov left a comment

Choose a reason for hiding this comment

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

Left a small comment about enabling it by default in run. Not sure if we should enable it in run at all. @colings86 what do you think? Otherwise looks good to me.

@@ -422,6 +422,7 @@ testClusters {
if (System.getProperty('run.distribution', 'default') == 'default') {
String licenseType = System.getProperty("run.license_type", "basic")
if (licenseType == 'trial') {
setting 'xpack.eql.enabled', 'true'
Copy link
Contributor

Choose a reason for hiding this comment

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

We enable it here only for trial, but we run integration tests under basic. That doesn't seem to be consistent. I would just put next to pack.sql.enabled until we figure out what to do with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to enable consistently with sql plugin for run task, seems logical.

@aleksmaus
Copy link
Contributor Author

Thought it would be convenient for development to have this enabled for gradle run task, especially since this is already done for sql plugin.

@colings86
Copy link
Contributor

on mobile so maybe missing something...

We should definitely have it enabled for the tests in the EQL module but not sure we need it enabled for all tests. We don’t do that for security AFAIK

@jasontedor what are you doing for Autoscaling? Just enable it in the Autoscaling tests?

@colings86
Copy link
Contributor

Oh I see we actually do it for security sorry. However I think we should match Autoscaling while it’s in development

@jasontedor
Copy link
Member

@colings86 We are only going to enable it where it's required in autoscaling tests. I don't think these should be enabled in run tasks (if someone wants them, -Dtests.es.xpack.eql.enabled=true would do it).

Copy link
Contributor

@imotov imotov 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

@costin costin left a comment

Choose a reason for hiding this comment

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

LGTM

@costin costin added the :Analytics/EQL EQL querying label Jan 23, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (:Search/EQL)

@aleksmaus aleksmaus merged commit 4b908b8 into elastic:feature/eql Jan 24, 2020
aleksmaus added a commit to aleksmaus/elasticsearch that referenced this pull request Jan 27, 2020
This was referenced Jan 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/EQL EQL querying
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants