Conversation
|
Should the tests be passing? |
|
@brettcannon not until Azure/msrest-for-python#31 is merged. If you can look into it as well, I'd like your review :) |
|
@yugangw-msft This version works great (I tested on "Azure Search" account creation, which has a super fast register/unregister). If you can review the algorithm it would be nice :) |
msrestazure/tools.py
Outdated
| session = kwargs['msrest']['session'] | ||
| url_prefix = _extract_subscription_url(r.request.url) | ||
| _register_rp(session, url_prefix, rp_name) | ||
| time.sleep(10) |
There was a problem hiding this comment.
any reason we want to sleep for 10 more seconds?
There was a problem hiding this comment.
no you're right, it was debugging, I'll remove it
msrestazure/tools.py
Outdated
| """Synchronously register the RP is paremeter.""" | ||
| post_url = "{}providers/{}/register?api-version=2016-02-01".format(url_prefix, rp_name) | ||
| get_url = "{}providers/{}?api-version=2016-02-01".format(url_prefix, rp_name) | ||
| _LOGGER.warning("Resource provider '%s' used by the command is not " |
There was a problem hiding this comment.
Maybe rename command to creating operation? I know CLI are commands, but guess not all of SDK's clients are commands
|
|
||
| def _register_rp(session, url_prefix, rp_name): | ||
| """Synchronously register the RP is paremeter.""" | ||
| post_url = "{}providers/{}/register?api-version=2016-02-01".format(url_prefix, rp_name) |
There was a problem hiding this comment.
I assume you have double checked that this api-version is available in sovereign clouds and stacks. I think so, but just in case. Or you can feel free to import azure.mgmt.resource, 99% chance, the package will be there, then you just create a client and use the sdk method. Up to you, since you know SDK better than anyone
There was a problem hiding this comment.
If I use the SDK, I have no reason to believe that latest of the SDK is available in this connection, and I don't know how to call Azure to ask what are the available ApiVersion.
I might do a list of ApiVersion and loop on it until I find one that is working.
Anyway, I will recheck the code with this kind of situation in mind, I guess I should check the status code and if it's not 200, just fail.
msrestazure/azure_configuration.py
Outdated
| if hasattr(self, 'hooks'): | ||
| self.hooks.append(register_rp_hook) | ||
| else: | ||
| _LOGGER.warning(("Your 'msrest' version is too old to activate all the ", |
There was a problem hiding this comment.
When I wrote warn, I got a Deprecation warning :)
Codecov Report
@@ Coverage Diff @@
## master #28 +/- ##
==========================================
+ Coverage 84.28% 85.04% +0.75%
==========================================
Files 6 7 +1
Lines 630 682 +52
==========================================
+ Hits 531 580 +49
- Misses 99 102 +3
Continue to review full report at Codecov.
|
| "registered. We are registering for you.", rp_name) | ||
| post_response = session.post(post_url) | ||
| if post_response.status_code != 200: | ||
| _LOGGER.warning("Registration failed. Please register manually.") |
There was a problem hiding this comment.
i suggest to include the rp_name in the warning
EDIT: ignore my comment please. We already mentioned it in the early warnings
|
@yugangw-msft @johanste |
6 participants
@yugangw-msft