try to get_role, but fall back to role from regex #305
Conversation
src/sagemaker/session.py
Outdated
| try: | ||
| role = self.boto_session.client('iam').get_role(RoleName=role_name)['Role']['Arn'] | ||
| except ClientError: | ||
| LOGGER.warning("Couldn't call 'get_role' to get Role ARN from role name {}.".format(role_name)) |
There was a problem hiding this comment.
I'd include a note here that this will exclude a role's path if it has one
There was a problem hiding this comment.
@andremoeller @laurenyu
AmazonSageMakerFullAccess that is a managed policy for SageMaker (see doc) does not allow iam:GetRole. The line in tryalways fail and goes to except if users attach only AmazonSageMakerFullAccess to the role; get_role() does not work. Could you consider another implementation for this?
There was a problem hiding this comment.
You're right -- in that case. The purpose of this is to attempt to get the role using iam:GetRole, but never to fail in this case -- not all users will only be using AmazonSageMakerFullAccess policy.
tests/unit/test_session.py
Outdated
| sess.boto_session.client('iam').get_role.side_effect = ClientError('Bad permissions!', {}) | ||
|
|
||
| actual = sess.get_caller_identity_arn() | ||
| assert actual == 'arn:aws:iam::369233609183:user/mia' |
There was a problem hiding this comment.
would it be possible to also test if the logger does warn?
…maker-python-sdk into get-execution-role-fix
Codecov Report
@@ Coverage Diff @@
## master #305 +/- ##
==========================================
+ Coverage 92.69% 92.75% +0.06%
==========================================
Files 50 50
Lines 3463 3466 +3
==========================================
+ Hits 3210 3215 +5
+ Misses 253 251 -2
Continue to review full report at Codecov.
|
Issue #, if available: #291
Description of changes:
Merge Checklist
Put an
xin 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.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.