[Core] Replace allow_broker with enable_broker_on_windows
#27726
Conversation
️✔️AzureCLI-FullTest
|
|
Hi @jiasli, |
️✔️AzureCLI-BreakingChangeTest
|
|
Core |
microsoft-github-policy-service
bot
added
Account
|
After some further consideration, I am now not inclined to expose a unified Reasons
|
I may agree with the choice, but not for the reasons that you brought up. MSAL and Azure CLI face different challenges. MSAL as a library can never know whether the calling app meets the prerequisite (i.e., registering the specific redirect URI) of a given platform, so, MSAL has to provide a per-platform opt-in flag, to avoid that "D-day, D+1 day, D+2 day" situation. (BTW, can you share me a link of where I mentioned a similar example before? I want to double check it.) Azure CLI as an app could go with only one In particular,
I do not think a user with The reason that a new |
This is true and this "D-day, D+1 day, D+2 day" situation is more severe on MSAL, as it raises an error, but on CLI side, even though there is no error, it is still a behavior change.
The link is right in AzureAD/microsoft-authentication-library-for-python#510 (comment): https://identitydivision.visualstudio.com/DevEx/_git/AuthLibrariesApiReview/pullrequest/6882?_a=files
I am actually not concerned about roaming across different CLI versions but roaming across different platforms for the same CLI version.
How about multiple users on different platforms? Consider your colleagues have
This is my opinion as well. More flexibility usually means more versatility and customizability (but sometimes also more complexity). |
yonzhan
modified the milestones:
November 2023 (2023-12-05),
December 2023 (2024-01-02)
jiasli
force-pushed
the
enable_broker_on_windows
branch
3 times, most recently
from
005189c
to
45d1a69
Compare
|
Telemetry-wise, another disadvantage of a combined In my opinion, the complexity introduced for a combined |
On Windows, after we make WAM the default login method, Also, after we kick off beta testing for broker support on mac, |
jiasli
requested review from
jsntcy,
kairu-ms,
zhoxing-ms and
necusjz
as code owners
|
When we start the Mac broker beta test, do we allow toggle whether broker on Mac is enabled, independent of whether the broker-on-Windows (i.e., WAM) is enabled or not? If so, we will need dedicated flags for Win and Mac, at least for now. This means a user will indeed be exposed to the details of configuring broker behavior differently per platform, regardless of how we name it. Specifically, the difference is really just:
Given that the per-platform discrepancy is already there (for better or worse), we think style 1 is more user friendly. |
jiasli
force-pushed
the
enable_broker_on_windows
branch
from
4f13544
to
3272723
Compare
jiasli
force-pushed
the
enable_broker_on_windows
branch
from
038c1d3
to
d466b26
Compare
| @@ -221,7 +221,7 @@ def _get_azure_cli_properties(self): | |||
| set_custom_properties(result, 'ShowSurveyMessage', str(self.show_survey_message)) | |||
| set_custom_properties(result, 'RegionInput', self.region_input) | |||
| set_custom_properties(result, 'RegionIdentified', self.region_identified) | |||
| set_custom_properties(result, 'AllowBroker', str(self.allow_broker)) | |||
| set_custom_properties(result, 'EnableBrokerOnWindows', str(self.enable_broker_on_windows)) | |||
There was a problem hiding this comment.
Initially I forgot to rename self.allow_broker to self.enable_broker_on_windows. This results in the below error in debug log:
cli.azure.cli.core.decorators: Suppress exception:
Traceback (most recent call last):
File "d:\cli\azure-cli\src\azure-cli\azure\cli\__main__.py", line 62, in <module>
raise ex
File "d:\cli\azure-cli\src\azure-cli\azure\cli\__main__.py", line 55, in <module>
sys.exit(exit_code)
SystemExit: 0
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "d:\cli\azure-cli\src\azure-cli-core\azure\cli\core\decorators.py", line 79, in _wrapped_func
return func(*args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^
File "d:\cli\azure-cli\src\azure-cli-core\azure\cli\core\telemetry.py", line 110, in generate_payload
cli = self._get_azure_cli_properties()
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "d:\cli\azure-cli\src\azure-cli-core\azure\cli\core\telemetry.py", line 224, in _get_azure_cli_properties
set_custom_properties(result, 'AllowBroker', str(self.allow_broker))
^^^^^^^^^^^^^^^^^
AttributeError: 'TelemetrySession' object has no attribute 'allow_broker'
telemetry.main: Split cli events and extra events failure: the JSON object must be str, bytes or bytearray, not NoneType
However, the CI doesn't find this. @evelyn-ys
There was a problem hiding this comment.
good catch. We should add telemetry check in CI too
|
Should we add this to changelog? As we have mentioned |
Nice catch and thanks for the reminder. As I initially aimed to merge this PR in 2023-12, but it was delayed to 2024-02. Even though it can be combined into I will submit another PR to update https://learn.microsoft.com/en-us/cli/azure/authenticate-azure-cli-web-account-manager. |
jiasli
changed the title
allow_broker with enable_broker_on_windowsallow_broker with enable_broker_on_windows
Related command
az loginDescription
This PR adopts the MSAL change AzureAD/microsoft-authentication-library-for-python#613 where MSAL will deprecate
allow_brokerand useenable_broker_on_windows.Testing Guide