[ARM] az group deployment create: Add parameter --aux-tenants to support cross tenants (#11765)

[zhoxing-ms]

Feature request: #11765

History Notes:
(Fill in the following template if multiple notes are needed, otherwise PR title will be used for history note.)

[Component Name 1] (BREAKING CHANGE:) (az command:) make some customer-facing change.
[Component Name 2] (BREAKING CHANGE:) (az command:) make some customer-facing change.

---

This checklist is used to make sure that common guidelines for a pull request are followed.

• The PR title and description has followed the guideline in Submitting Pull Requests.

• I adhere to the Command Guidelines.

[yonzhan]

add to S166.

[jiasli]

I'd prefer to error out when both are specified, as giving warning instead encourages incorrect usage.

azure-cli/src/azure-cli-core/azure/cli/core/_profile.py

Lines 604 to 605 in 5edbd82

	if subscription and tenant:
	raise CLIError("Please specify only one of subscription and tenant, not both")

[zhoxing-ms]

It has been verified in the validator and an error has been returned for this scenario.
This method is a underlying method, which is also called in many other places. Just logging warning will have a smaller impact on different businesses, unless we can confirm that all scenarios calling it in the future can directly report an error.

[jiasli]

In other places, no invocation involves aux_tenants. Making them mutual-exclusive won't cause any breaking change.

[zhoxing-ms]

Yes, I know it has no effect on the existing calling methods. I mean, I didn't know if future calls should be waring or error.
I think you're right. I'll change the logic.

[jiasli]

Suggested change

	if isinstance(aux_tenants, list):
	if aux_tenants:

[qianwens]

or []) [](start = 61, length = 7)

if both ext_sub and ext_tenants are empty, the original code append sub = self.get_subscription([]), your code removed this logic, please check if it will cause regression

[zhoxing-ms]

In the original code, when aux_subscriptions is empty, ext_subs is [], sub = self.get_subscription (ext_sub) will not be executed.
I understand that the previous code was written this way to prevent aux_subscriptions from having null pointer problems.

[qianwens]

[jiasli]

Let's add some tests for the change at

azure-cli/src/azure-cli-core/azure/cli/core/tests/test_profile.py

Line 548 in 34549d2

def test_get_login_credentials_aux_subscriptions(self, mock_get_token, mock_read_cred_file):

[zhoxing-ms]

Let's add some tests for the change at

done

[jiasli]

Nice work!

[Juliehzl]

Please use + not * to avoid [] for aux_tenants and aux_subs

[Juliehzl]

Can you consider abstracting this part to a new test function?

[zhoxing-ms]

This question has been considered before, my thoughts are:
The name of this method is test_group_deployment_crosstenant(), but the original test was only for cross aux_sub. Now I've added the test logic for cross aux_tenant, which is consistent with the method name.
My original plan was to directly delete the logic of testing aux_sub in test_group_deployment_crosstenant() after deprecating --aux-sub. So that some common code can be reused and the test with two parameters bound together can be implemented properly.
What do you think about it?

[zhoxing-ms]

And I will add a new test function for the refactored code, so I think it might be better to use only a function for the old code to distinguish the tests of the old code from the new code.

[Juliehzl]

Another concern here is that when PR #10751 merged, you may need to re-apply your commands change to all new commands in #10751.

[zhoxing-ms]

Another concern here is that when PR #10751 merged, you may need to re-apply your commands change to all new commands in #10751.

Good suggestions! I will submit a dedicated commit to support cross tenant for the refactored code.

[Tiano2017]

group deployment create [](start = 18, length = 23)

I don't see test recordings for this. Is it not needed for this change?

[zhoxing-ms]

Because this case is not easy to write as rerecord, so now it is a live_only mode.