Conversation
|
aks-preview |
src/aks-preview/azext_aks_preview/tests/latest/test_aks_commands.py
Outdated
Show resolved
Hide resolved
src/aks-preview/azext_aks_preview/tests/latest/test_aks_commands.py
Outdated
Show resolved
Hide resolved
FumingZhang
left a comment
There was a problem hiding this comment.
LGTM, requesting @andyliuliming for another review.
|
|
||
|
|
||
| def aks_addon_show_table_format(result): | ||
| def parser(entry): |
There was a problem hiding this comment.
why do we need a internal function here?
There was a problem hiding this comment.
I had that just to keep the formatting consistent with the other functions. It is also possible that if in the future we allow for multiple addons to be shown at once, the internal function will be useful to process each of them.
| if assignee.find('@') >= 0: # looks like a user principal name | ||
| result = list(client.users.list( | ||
| filter="userPrincipalName eq '{}'".format(assignee))) | ||
| if not result: |
There was a problem hiding this comment.
Hi Andy, a lot of this code is not mine. It was simply moved around for organization purposes and to avoid circular dependencies. I ensured everything still works, but did not change any code functionality except for functions that were directly related to addons (and there were only 2 of them). I'm not sure it would be safe for me to change the functionality of these other functions without knowing there purpose.
This one seems easy enough to add in, however, if you want me to, though it might break things if result could simply be a False value for whatever reason.
There was a problem hiding this comment.
On second thought, I think it is not result because result is a list, so if the list is empty, not result would evaluate to true.
| if not result: | ||
| result = list(client.service_principals.list( | ||
| filter="servicePrincipalNames/any(c:c eq '{}')".format(assignee))) | ||
| if not result: # assume an object id, let us verify it |
There was a problem hiding this comment.
Hi Andy, a lot of this code is not mine. It was simply moved around for organization purposes and to avoid circular dependencies. I ensured everything still works, but did not change any code functionality except for functions that were directly related to addons (and there were only 2 of them). I'm not sure it would be safe for me to change the functionality of these other functions without knowing there purpose.
This one seems easy enough to add in, however, if you want me to, though it might break things if result could simply be a False value for whatever reason.
There was a problem hiding this comment.
On second thought, I think it is not result because result is a list, so if the list is empty, not result would evaluate to true.
| value=0.1 * x, total_val=1.0) | ||
| try: | ||
| # TODO: break this out into a shared utility library | ||
| create_role_assignment( |
There was a problem hiding this comment.
why not call _create_role_assignment?
There was a problem hiding this comment.
Hi Andy, a lot of this code is not mine. It was simply moved around for organization purposes and to avoid circular dependencies. I ensured everything still works, but did not change any code functionality except for functions that were directly related to addons (and there were only 2 of them). I'm not sure it would be safe for me to change the functionality of these other functions without knowing there purpose.
But I remember for this one, the original code had both a _create_role_assignment and a create_role_assignment and arbitrarily picked between the 2 for various uses.
There was a problem hiding this comment.
I consolidated everything into create_role_assignment.
| break | ||
| logger.info(ex.message) | ||
| except: # pylint: disable=bare-except | ||
| pass |
There was a problem hiding this comment.
Hi Andy, a lot of this code is not mine. It was simply moved around for organization purposes and to avoid circular dependencies. I ensured everything still works, but did not change any code functionality except for functions that were directly related to addons (and there were only 2 of them). I'm not sure it would be safe for me to change the functionality of these other functions without knowing their purpose.
| cli_ctx, role, service_principal_msi_id, is_service_principal, scope=scope) | ||
| break | ||
| except CloudError as ex: | ||
| if ex.message == 'The role assignment already exists.': |
There was a problem hiding this comment.
is there any error code we can use instead of checking the error message?
There was a problem hiding this comment.
Hi Andy, a lot of this code is not mine. It was simply moved around for organization purposes and to avoid circular dependencies. I ensured everything still works, but did not change any code functionality except for functions that were directly related to addons (and there were only 2 of them). I'm not sure it would be safe for me to change the functionality of these other functions without knowing there purpose.
| @@ -0,0 +1,900 @@ | |||
| # -------------------------------------------------------------------------------------------- | |||
There was a problem hiding this comment.
I suppose this file is copied from some where?
I would suggest break out one new PR for the file renaming or function name renaming.
that will helps a lot for the code reviewers.
There was a problem hiding this comment.
A few files were newly made to bring better organization into the codebase. However, functions were only moved out of necessity (e.g. to avoid circular dependencies with the custom.py file) and thus are related to the original addon changes I made. Also, functions were only renamed to switch them from private to public (removing the underscore at the beginning of the name). Consequently, I believe a separate PR wouldn't make sense. Please advise if you believe otherwise.
|
@IshanArya Could you please address those CI issues and conflicts? |
|
@IshanArya Any update? |
|
Any update on this? |
|
Because this PR has not been updated for a long time, so I close it first. When we need merge it again, then reopen it |
|
Thanks @zhoxing-ms , the owner is not working on this project now. I will create a new PR instead. |
This checklist is used to make sure that common guidelines for a pull request are followed.
General Guidelines
azdev style <YOUR_EXT>locally? (pip install azdevrequired)python scripts/ci/test_index.py -qlocally?About Extension Publish
There is a pipeline to automatically build, upload and publish extension wheels.
Once your PR is merged into master branch, a new PR will be created to update
src/index.jsonautomatically.The precondition is to put your code inside this repo and upgrade the version in the PR but do not modify
src/index.json.