-
Notifications
You must be signed in to change notification settings - Fork 40
Add Integration Tests for E2E Multi-type transformations #1354
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
Add Integration Tests for E2E Multi-type transformations #1354
Conversation
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1354 +/- ##
============================================
+ Coverage 79.91% 80.12% +0.20%
Complexity 2977 2977
============================================
Files 443 440 -3
Lines 16217 16188 -29
Branches 1087 1078 -9
============================================
+ Hits 12960 12970 +10
+ Misses 2617 2583 -34
+ Partials 640 635 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Tanner Lewis <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took a brief look, added some high level comments.
...apture/dockerSolution/src/main/docker/migrationConsole/lib/integ_test/integ_test/conftest.py
Show resolved
Hide resolved
...ution/src/main/docker/migrationConsole/lib/integ_test/integ_test/elasticsearch_operations.py
Show resolved
Hide resolved
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
...on/src/main/docker/migrationConsole/lib/integ_test/integ_test/test_cases/multi_type_tests.py
Show resolved
Hide resolved
...on/src/main/docker/migrationConsole/lib/integ_test/integ_test/test_cases/multi_type_tests.py
Show resolved
Hide resolved
index_dict[index_name]['index'] = index_name | ||
return index_dict | ||
|
||
def check_doc_counts_match(self, cluster: Cluster, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: This is a little bit weird that you have a verify function above that throws and a check function here that fails the test case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes agreed, this was copied from the existing integ test common_operations.py
file that got removed. Once we remove the existing integration tests I'd like to remove passing the test case to any of these functions and allow them to just throw an error if a check fails.
|
||
# Cleanup generated transformation files | ||
try: | ||
shutil.rmtree("/shared-logs-output/test-transformations") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how much effort would it be to delete these when we're done and only upon success? Adding and remembering some uniqifier (like timestamp) could be helpful too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably a small effort, especially if the teardown function has some way of determining success or failure. It seems like we really want a lot of things to get generated somewhere persistent, such as service logs, cluster logs, test_output, etc. Currently items aren't cleaned up until another test case is executed.
run_on_all_cases(test_cases=test_cases, operation=lambda case: case.perform_initial_operations()) | ||
control_test_case.perform_snapshot_create() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ni: consistent indenting for all of the following lines would make it easier to quickly understand the whole block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed will update
|
||
|
||
def get_operations_library_by_version(version: ClusterVersion): | ||
if is_incoming_version_supported(limiting_version=ElasticsearchV5_X, incoming_version=version): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish that we could setup numeric values for versions, but because of the reset and fork, it's a lot harder to manage (and would have some exceptional cases).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it seems we have ended up on this ES|OS_x.y
pattern
|
||
class Test0002IndexWithNoDocumentsMetadataMigration(MATestBase): | ||
def __init__(self, console_config_path: str, console_link_env: Environment, unique_id: str): | ||
allow_combinations = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit/future: it might be better to model these as a function/lambda. These seems like they could start to get pretty when big fully materialized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My next iteration was going to be to model this as (source_version, list(target_version))
, but curious what you had in mind for a lambda around this?
endpoint: "http://ma-capture-proxy:9200" | ||
directEndpoint: "http://elasticsearch-master:9200" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we have this split now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The capture proxy will need both of these values. In the future I also think the Migration Console will want the ability to hit the source directly or hit the capture proxy, we don't currently have support for this yet though
allowRuntimeOverride: true | ||
docTransformerConfigFile: | ||
source: parameterConfig | ||
value: "/shared-logs-output/test-transformations/transformation.json" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See https://opensearch.atlassian.net/browse/MIGRATIONS-2401 for a better way to send configs/arguments into processes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I expect this pattern to change when we have support for modifying configmaps as part of a workflow
@@ -1,3 +1,5 @@ | |||
sharedLogsVolumeEnabled: false | |||
sharedLogsPvc: "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm - this isn't very obvious (to clear the pvc) - what happens if this isn't done?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean. As a default we will not mount any shared logs pvc, but our MA chart will fill this out to mount the shared logs pvc
} | ||
|
||
pause() { | ||
kill_minikube_processes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When do you typically do this in your workflows? I usually leave mine running and don't think much about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only when I need to restart normally. Really this is just a means for cleaning up if a user wants to.
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
… tests Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
Description
This change accomplishes a few high level tasks:
Adds two new E2E test cases for Union Multi-type Transformation and Split Multi-type Transformation
Adds shared logs volume and mounts to relevant pods for K8s
Issues Resolved
https://opensearch.atlassian.net/browse/MIGRATIONS-2340
Testing
Local K8s testing for new model
Cloud testing existing tests
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.