-
Notifications
You must be signed in to change notification settings - Fork 27
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
Updates to Orchestrator API for OSI #805
Conversation
…ine pattern, add assume role for all actions Signed-off-by: Tanner Lewis <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #805 +/- ##
==========================================
+ Coverage 88.18% 88.45% +0.26%
==========================================
Files 56 67 +11
Lines 3478 3723 +245
==========================================
+ Hits 3067 3293 +226
- Misses 411 430 +19
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
# Conflicts: # TrafficCapture/dockerSolution/src/main/docker/migrationConsole/lib/console_link/tests/test_osi_utils.py
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]>
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.
A few questions, but no major concerns
- uses: codecov/codecov-action@v4 | ||
with: | ||
files: ./coverage.xml | ||
|
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.
Note for a follow up change in the sonarqube stuff to incorporate this
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.
Created a JIRA issue: Add Console API test coverage to Sonarqube
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.
Thanks for creating this, yes we need to add
@@ -13,6 +13,8 @@ sqlparse = "==0.5.0" | |||
pyopenssl = "*" | |||
|
|||
[dev-packages] | |||
coverage = "*" | |||
moto = "*" |
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 looks like you can install moto with just the specific dependencies you want to mock (https://docs.getmoto.org/en/latest/docs/getting_started.html#installing-moto) -- worth doing? I assume we're mocking a pretty narrow list
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.
Went ahead and added the specifier for sts
for moto, from my testing all decorators like mock_sts
have been removed in favor of using the single mock_aws
} | ||
|
||
|
||
# Moto has not yet implemented mocking for OSI API actions, using @patch for these calls |
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.
So moto is basically handling/mocking creating the boto3 client? It looks like we could add the OSI actions directly to moto (like https://docs.getmoto.org/en/latest/docs/services/patching_other_services.html or via PR to their repo), but I think this approach is fine with me for 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.
Switched to using patch for moto like you mentioned to give a little more flexibility here. Thanks for suggestion
create_pipeline_from_json(osi_client=determine_osi_client(request_data=request_data, action_name=action_name), | ||
input_json=request_data, | ||
pipeline_template_path=PIPELINE_TEMPLATE_PATH) | ||
except InvalidAuthParameters as i: |
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.
Minor note that at some point, it would be great to have a test that exercised this branch, but not a blocker for this PR
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.
Have added a test for this now 👍
def osi_start_migration(request): | ||
return osi_update_workflow(request=request, osi_action_func=start_pipeline, action_name='Start') | ||
|
||
|
||
@api_view(['POST']) | ||
@parser_classes([JSONParser]) | ||
def osi_stop_migration(request): | ||
request_data = request.data | ||
logger.info(pretty_request(request, request_data)) | ||
return osi_update_workflow(request=request, osi_action_func=stop_pipeline, action_name='Stop') |
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.
Is there/should there be validation that the pipeline exists for these actions? What happens if it doesn't?
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 imagine users will get a 500 with the error message, that the pipeline doesn't exist. But agree this is likely a common error case we should test for and be more specific for like a 404 response code. Have created a task around this here: https://opensearch.atlassian.net/browse/MIGRATIONS-1877
@@ -57,6 +58,20 @@ def __init__(self, config: Dict) -> None: | |||
self.tags = convert_str_tags_to_dict(tags) | |||
|
|||
|
|||
def get_assume_role_session(role_arn, session_name) -> boto3.Session: |
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'd love to see an example of what the caller sees if this fails. It seems like one of the more likely to fail pieces, given the general complication of IAM stuff.
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.
From my testing, users will get a 500 and see the boto3 error message along the lines of Unable to assume role: .... for x reason
. Which may be clear enough for now, but potentially something we could make more obvious
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
Description
This changes introduces the following additions to the Console Orchestrator API
Issues Resolved
https://opensearch.atlassian.net/browse/MIGRATIONS-1843
Testing
Unit testing, Cloud testing
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.