{Core} Refactor code for passing arguments #30300
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
️✔️AzureCLI-FullTest
|
|
Hi @jiasli, |
️✔️AzureCLI-BreakingChangeTest
|
|
Thank you for your contribution! We will review the pull request and get back to you soon. |
microsoft-github-policy-service
bot
added
Account
jiasli
commented
Nov 11, 2024
| @@ -665,7 +666,7 @@ def _create_credential(self, account, tenant_id=None, client_id=None): | |||
| """ | |||
| user_type = account[_USER_ENTITY][_USER_TYPE] | |||
| username_or_sp_id = account[_USER_ENTITY][_USER_NAME] | |||
| tenant_id = tenant_id if tenant_id else account[_TENANT_ID] | |||
| tenant_id = tenant_id or account[_TENANT_ID] | |||
There was a problem hiding this comment.
https://docs.python.org/3/library/stdtypes.html#boolean-operations-and-or-not
x or y: if x is true, then x, else y
This was referenced Nov 11, 2024
jiasli
requested review from
evelyn-ys,
calvinhzy and
bebound
as code owners
jiasli
force-pushed
the
refactor-args
branch
from
0f28626 to
d2707ea
Compare
jiasli
commented
Nov 14, 2024
| @@ -413,7 +413,7 @@ def get_login_credentials(self, resource=None, client_id=None, subscription_id=N | |||
| credential = self._create_credential(account, client_id=client_id) | |||
| external_credentials = [] | |||
| for external_tenant in external_tenants: | |||
| external_credentials.append(self._create_credential(account, external_tenant, client_id=client_id)) | |||
| external_credentials.append(self._create_credential(account, tenant_id=external_tenant)) | |||
There was a problem hiding this comment.
_create_credential() defines tenant_id as a keyword argument:
Calling _create_credential() should follow the same pattern.
jiasli
commented
Nov 14, 2024
| @@ -938,7 +938,7 @@ def _transform_subscription_for_multiapi(s, s_dict): | |||
| s_dict[_MANAGED_BY_TENANTS] = [{_TENANT_ID: t.tenant_id} for t in s.managed_by_tenants] | |||
|
|
|||
|
|
|||
| def _create_identity_instance(cli_ctx, *args, **kwargs): | |||
| def _create_identity_instance(cli_ctx, authority, tenant_id=None): | |||
There was a problem hiding this comment.
Very limited args (actually only one - authority) and kwargs are used:
It's better to be explicit.
https://peps.python.org/pep-0020/
Explicit is better than implicit.
jiasli
force-pushed
the
refactor-args
branch
from
d2707ea to
b12ac18
Compare
jiasli
commented
Nov 14, 2024
| @@ -362,12 +362,10 @@ def logout_all(self): | |||
| identity.logout_all_users() | |||
| identity.logout_all_service_principal() | |||
|
|
|||
| def get_login_credentials(self, resource=None, client_id=None, subscription_id=None, aux_subscriptions=None, | |||
| aux_tenants=None): | |||
| def get_login_credentials(self, resource=None, subscription_id=None, aux_subscriptions=None, aux_tenants=None): | |||
There was a problem hiding this comment.
client_id should not be supported by get_login_credentials().
bebound
reviewed
Nov 14, 2024
| @@ -655,18 +653,17 @@ def _try_parse_msi_account_name(account): | |||
| return parts[0], (None if len(parts) <= 1 else parts[1]) | |||
| return None, None | |||
|
|
|||
| def _create_credential(self, account, tenant_id=None, client_id=None): | |||
| def _create_credential(self, account, tenant_id=None): | |||
There was a problem hiding this comment.
client_id is used in here, need to be reverted in the future: https://github.com/Azure/azure-cli/pull/30302/files#diff-242cdb28a28d3ff1d8467d2416389e39c0d149dab437550d6a6e88cca877c220R461
There was a problem hiding this comment.
There is no guarantee when #30302 will be merged. Better to keep the code clean for now.
There was a problem hiding this comment.
If there's possibility that client_id is needed in the future, I agree with Hang that we can keep it for now. There's no harm to leave it as it is, right?
There was a problem hiding this comment.
Ok, changed as requested, but client_id will only be supported by get_raw_token(), not get_login_credentials(), so I am still removing it from get_login_credentials().
bebound
previously approved these changes
Nov 14, 2024
jiasli
force-pushed
the
refactor-args
branch
from
b12ac18 to
5decba3
Compare
jiasli
force-pushed
the
refactor-args
branch
from
5decba3 to
4fc8040
Compare
evelyn-ys
approved these changes
Nov 14, 2024
bebound
approved these changes
Nov 14, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related command
az loginDescription
Refactor code for passing arguments.
Pave way for
az account get-access-token: Add--client-idargument #30301