-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
feat: add cross-account runbook + actions #4802
base: default
Are you sure you want to change the base?
Conversation
decision = input("Need to import SM profiles from a X-account? [y/n]: ") | ||
if decision == "y": | ||
x_account_role = input( | ||
"X-account role ARN to assume? This would be the account that originally created" |
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.
"X-account" -> terminology that customers would understand better. i.e.
"Need to import SageMaker profiles from a different account?"
"Secondary account role ARN to assume"
what account is this asking for exactly? the DataZone Domain account or the SageMaker Domain account? i.e. above line mentions "SM profiles" but below mentions "DataZone domain"
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.
also avoid abbreviating SageMaker as SM to avoid potential user confusion
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.
Ack. Will clean this up.
self.dz_client = session.client( | ||
"datazone", region_name=region, endpoint_url=dz_endpoint_url | ||
) | ||
self.byod_client = session.client( | ||
"datazone-byod", region_name=region, endpoint_url=dz_endpoint_url | ||
) |
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 clear where these are used - looks like this is overwriting the clients - do we need both? (clients for DZ domain account and SM domain account)
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.
Yea. These override the existing clients because we need to assume a new role/session. I think we can extend these with a new var so that the original WF isnt touched.
Issue #, if available:
Description of changes:
Testing done:
Manually using hulk test accounts
Merge Checklist
Put an
x
in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.python3 -m black -l 100 {path}/{notebook-name}.ipynb
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.