Add support for service name aliases and alias sagemaker runtime #1334
Conversation
joguSD
requested review from
dstufft,
jamesls,
stealthycoin,
JordonPhillips and
kyleknap
Codecov Report
@@ Coverage Diff @@
## develop #1334 +/- ##
===========================================
+ Coverage 98.12% 98.12% +<.01%
===========================================
Files 46 46
Lines 7694 7699 +5
===========================================
+ Hits 7550 7555 +5
Misses 144 144
Continue to review full report at Codecov.
|
|
|
||
| # service_name => alias | ||
| ALIAS_CASES = { | ||
| 'sagemaker-runtime': 'runtime.sagemaker' |
There was a problem hiding this comment.
What about making _SERVICE_NAME_ALIASES public and dynamically creating the reverse map here?
Otherwise we'll have to remember to update this test if we add a new alias.
There was a problem hiding this comment.
My initial thought was that having at least a small change to the tests would be good, but I can also see the merit in getting the tests automatically too. And rather than reversion the map I think I'll just make that one public and import it. While it should never happen a service name could have multiple aliases so alias => service_name makes more sense.
| if responses is None: | ||
| responses = [] | ||
|
|
||
| emitter = mock.Mock() |
There was a problem hiding this comment.
Looks like this causes issues on Python 2, gonna leave the mock without the spec as that's how it was before.
There was a problem hiding this comment.
Did some more digging on why this was happening. We apply a copy.copy on the event_emitter here which seems to cause issues on Python 2. What appears to happen is the copy becomes an actual instance of the specified class that never had its constructor called (and thus can be missing attributes).
There was a problem hiding this comment.
Looks good. Another thing that we may want to update is the invalid service name test: https://github.com/boto/botocore/blob/develop/tests/functional/test_service_names.py. We were thinking about removing it from the blacklist, but this PR had not been out when it first got merged.
joguSD
force-pushed
the
sagemaker-runtime
branch
2 times, most recently
from
3774b8d to
24fe394
Compare
joguSD
force-pushed
the
sagemaker-runtime
branch
from
24fe394 to
d647386
Compare
Must be merged in conjunction with: aws/aws-cli#3019
This renames
runtime.sagemakertosagemaker-runtimeand adds the ability to alias service names for backwards compatibility reasons.