[ACR] Add tests for foreign cloud #19271
Conversation
sdk/containerregistry/azure-containerregistry/tests/testcase.py
Outdated
Show resolved
Hide resolved
|
/azp run python - containerregistry - tests-weekly |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run python - containerregistry - tests-weekly |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run python - containerregistry - tests-weekly |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| return AzureAuthorityHosts.AZURE_GOVERNMENT | ||
| raise ValueError("Endpoint ({}) could not be understood".format(endpoint)) | ||
|
|
||
| def get_credential_scopes(self, authority): |
There was a problem hiding this comment.
We should not publish this as a sample. We do not want customers to have this code in their applications.
There was a problem hiding this comment.
Is there a best practices for demonstrating how to authenticate when in a sovereign cloud? or another sample that could demonstrate this
There was a problem hiding this comment.
There was a problem hiding this comment.
We've also decided not to use this approach because if customers have custom endpoints (vanity URLs) in national clouds, this would not give the desired result. That's why we're letting them pass the scope directly (in this version) -- it's because we need this to know which cloud we're in if they have a custom endpoint.
There was a problem hiding this comment.
@annelo-msft is the "this approach" you're referring to the approach demonstrated in the samples or in the clouddiscover.md document?
There was a problem hiding this comment.
In the sample ... I must not have refreshed because I didn't see @johanste's second comment. Apologies!
There was a problem hiding this comment.
Removed all of this, this will need to be addressed in the future
| @@ -74,7 +74,7 @@ def get_refresh_token(self, service, **kwargs): | |||
| def exchange_aad_token_for_refresh_token(self, service=None, **kwargs): | |||
| # type: (str, Dict[str, Any]) -> str | |||
| refresh_token = self._client.authentication.exchange_aad_access_token_for_acr_refresh_token( | |||
| service=service, access_token=self._credential.get_token(self.credential_scope).token, **kwargs | |||
| service=service, access_token=self._credential.get_token(*self.credential_scopes).token, **kwargs | |||
There was a problem hiding this comment.
I don't know Python very well ... what does the * before self.credential_scopes do?
| """ | ||
|
|
||
| def __init__(self, endpoint, credential, **kwargs): | ||
| # type: (str, Optional[TokenCredential], Dict[str, Any]) -> None | ||
| auth_policy = ContainerRegistryChallengePolicy(credential, endpoint, **kwargs) | ||
| audience = kwargs.pop("audience", ["https://management.core.windows.net/.default"]) |
There was a problem hiding this comment.
You actuall don't use or store this variable in this constructor
| azure-identity | ||
| msrestazure>=0.4.11 |
There was a problem hiding this comment.
Why would you need msrestazure is any way?
There was a problem hiding this comment.
I need it for tests. The best way I could get to change the management client to target foreign clouds used msrestazure.
closes #18608