[ACS] Update the handling of service principals.#1469
[ACS] Update the handling of service principals.#1469derekbekoe merged 1 commit intoAzure:masterfrom
Conversation
|
@brendandburns, thanks for your PR! By analyzing the history of the files in this pull request, we identified @sanar-microsoft and @colemickens to be potential reviewers. |
colemickens
left a comment
colemickens
left a comment
There was a problem hiding this comment.
one small question out of curiosity.
There was a problem hiding this comment.
What's the purpose of calling store_acs_service_principal twice (lines 274, 280)?
There was a problem hiding this comment.
removed, I think originally I didn't salt the URL, so if I didn't save it it prevented subsequent creates, but that was removed, so it's no longer necessary...
derekbekoe
changed the title
|
Comment addressed, please re-check. Thanks! |
derekbekoe
left a comment
derekbekoe
left a comment
There was a problem hiding this comment.
On it's own, this change LGTM but command modules should not have dependencies on each other as they version independently and can change.
| from azure.cli.command_modules.role.custom import ( | ||
| _graph_client_factory, | ||
| show_service_principal, | ||
| ) |
There was a problem hiding this comment.
Command modules should not have dependencies on each other...
Since command modules are/will be versioned independently, this could fail in the future if there's a change to the 'role' module that changes these methods (especially as the _graph_client_factory() is private).
Any logic needed should be included here.
You can choose to leave it as is but there are no guarantees.
4 participants
Fix a bug where command line flags would be ignored
Validate that the service principal still exists
@colemickens for sanity checking
Fixes #1442