Skip to content

Skip by default compareResults in pipeline tests for serverless #1521

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Oct 24, 2023
Merged

Skip by default compareResults in pipeline tests for serverless #1521

merged 4 commits into from
Oct 24, 2023

Conversation

@mrodm
Copy link
Contributor

@mrodm mrodm commented Oct 23, 2023

This PR skips the compare results function for serverless by default. This function is in charge of comparing the documents received by elasticsearch with the samples. The issue is in Serverless, since there are fields related to Geo IP database that cannot be checked properly.

It also adds a new environment variable ELASTIC_PACKAGE_SERVERLESS_PIPELINE_TEST_ENABLE_COMPARE_RESULTS. This new environment variable can be defined to force to run this compareResults function.

When elastic-package test pipeline is executed with a local stack, there are no changes. The complete pipeline tests are run.

Examples to execute with this new environment variable:

 $ elastic-package stack up -v -d --version 8.10.4 --provider serverless

 $ ELASTIC_PACKAGE_SERVERLESS_PIPELINE_TEST_ENABLE_COMPARE_RESULTS=true elastic-package test pipeline -v

 $ ELASTIC_PACKAGE_SERVERLESS_PIPELINE_TEST_ENABLE_COMPARE_RESULTS=false elastic-package test pipeline -v

@mrodm mrodm requested a review from a team October 23, 2023 14:47
@mrodm mrodm self-assigned this Oct 23, 2023
@mrodm mrodm mentioned this pull request Oct 23, 2023
Merged
7 tasks
mrodm
mrodm commented Oct 23, 2023
View reviewed changes
internal/testrunner/runners/pipeline/test_result.go
Comment on lines -27 to -48
var geoIPKeys = []string{
"as",
"geo",
"client.as",
"client.geo",
"destination.as",
"destination.geo",
"host.geo", // not defined host.as in ECS
"observer.geo", // not defined observer.as in ECS
"server.as",
"server.geo",
"source.as",
"source.geo",
"threat.enrichments.indicateor.as",
"threat.enrichments.indicateor.geo",
"threat.indicateor.as",
"threat.indicateor.geo",
// packages using geo fields in nested objects
"netskope.alerts.user.geo",
"netskope.events.user.geo",
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed since in this PR it is removed the comparison of documents with the samples.

jsoriano
jsoriano approved these changes Oct 23, 2023
View reviewed changes
Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

👍

internal/testrunner/runners/pipeline/runner.go Outdated
)

var (
serverlessEnableCompareResults = environment.WithElasticPackagePrefix("SERVERLESS_PIPELINE_TEST_ENABLE_COMPARE_RESULTS")
Copy link
Member

Choose a reason for hiding this comment

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

Add somewhere a TODO comment mentioning that this is a temporary workaround till we have something better for deterministic geoip in serverless.

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 the TODO message here: 6b35523

jsoriano
jsoriano approved these changes Oct 23, 2023
View reviewed changes
Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

👍

jsoriano
jsoriano reviewed Oct 23, 2023
View reviewed changes
internal/testrunner/runners/pipeline/runner.go Outdated
)

var (
serverlessEnableCompareResults = environment.WithElasticPackagePrefix("SERVERLESS_PIPELINE_TEST_ENABLE_COMPARE_RESULTS")
Copy link
Member

Choose a reason for hiding this comment

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

I am thinking now that maybe we should leave this enabled by default, so when running locally, by default we see the errors. And we can disable in the CI jobs where we want to use this. WDYT?

Suggested change
serverlessEnableCompareResults = environment.WithElasticPackagePrefix("SERVERLESS_PIPELINE_TEST_ENABLE_COMPARE_RESULTS")
serverlessEnableCompareResults = environment.WithElasticPackagePrefix("SERVERLESS_PIPELINE_TEST_DISABLE_COMPARE_RESULTS")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, agreed
That would make the default behavior to run those comparison and the exception disable them. It makes sense for me. I'll change it.

@mrodm mrodm requested a review from jsoriano October 24, 2023 08:29
jsoriano
jsoriano approved these changes Oct 24, 2023
View reviewed changes
Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

👍

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @mrodm

@mrodm mrodm merged commit a68f5ba into elastic:main Oct 24, 2023
@mrodm mrodm deleted the skip_compare_results_serverless_pipeline_tests branch October 24, 2023 11:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jsoriano jsoriano jsoriano approved these changes

Assignees

@mrodm mrodm

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mrodm @elasticmachine @jsoriano