rbac: improve 'create-for-rbac' #1499
Conversation
yugangw-msft
force-pushed
the
more_log
branch
from
b72f7ff to
62a6c4a
Compare
|
//cc: @derekbekoe @tjprescott |
| def _create_role_assignment(role, assignee, resource_group_name=None, scope=None, | ||
| ocp_aad_session_key=None): | ||
| def _create_role_assignment(role, assignee, resource_group_name=None, scope=None, #pylint: disable=too-many-arguments | ||
| ocp_aad_session_key=None, resolve_assignee=True): |
There was a problem hiding this comment.
Should we have help for this parameter?
There was a problem hiding this comment.
this is an internal method which is not exposed as a command
| @@ -399,18 +399,22 @@ def _build_application_creds(password=None, key_value=None, key_type=None,#pylin | |||
| def create_service_principal(identifier): | |||
| return _create_service_principal(identifier) | |||
|
|
|||
| def _create_service_principal(identifier, retain_raw_response=False): | |||
| def _create_service_principal(identifier, retain_raw_response=False, resolve_app=True): | |||
There was a problem hiding this comment.
Same like my previous comment
| import time | ||
| graph_client = _graph_client_factory() | ||
| role_client = _auth_client_factory().role_assignments | ||
| role = role or 'contributor' |
There was a problem hiding this comment.
The parameter default is Contributor. Does the casing matter?
Actually, do you need this or statement if there is a default set at the top?
There was a problem hiding this comment.
Casing doesn't matter, but yes this code is redundant as we have a default set at the top
| time.sleep(5) | ||
| else: | ||
| logger.warning("Creating service principal failed for appid '%s'. Trace followed:\n%s", | ||
| name, ex.response.headers) #pylint: disable=no-member |
There was a problem hiding this comment.
Should we log the detailed trace to logger.debug and then raise CLIError("Creating service principal failed for appid '%s'.").
It appears that logging a warning and then calling raise would print the full stacktrace to the console?
There was a problem hiding this comment.
Let us do it later.
For now, logging as a warn by default will display the correlation id to give it to the service team to investigate. Using --debug changes the timing and hence it becomes hard to reproduce.
| except Exception as ex: #pylint: disable=broad-except | ||
| #pylint: disable=line-too-long | ||
| if 'The appId of the service principal does not reference a valid application object' in str(ex): | ||
| time.sleep(5) |
There was a problem hiding this comment.
Consider: Add logger.warning message indicating that it is retrying something and what retry number you're currently on e.g. (Retry 3/12).
| break | ||
| except Exception as ex: | ||
| if ' does not exist in the directory ' in str(ex): | ||
| time.sleep(5) |
There was a problem hiding this comment.
Again, suggest retry warning messages so user is informed.
|
@derekbekoe, please take another look. |
|
@yugangw-msft Have the changes been pushed here? |
|
@derekbekoe, sorry, I just now pushed. |
Fix #1190, Fix #1332
Includes:
//cc: @colemickens, @ahmetalpbalkan