Skip to content

[ACR] Fixing credential_scopes kwarg - #19664

Merged
Sean Kane (seankane-msft) merged 7 commits into
Azure:mainfrom
seankane-msft:acr-credscope
Jul 6, 2021
Merged

[ACR] Fixing credential_scopes kwarg#19664
Sean Kane (seankane-msft) merged 7 commits into
Azure:mainfrom
seankane-msft:acr-credscope

Conversation

@seankane-msft

Copy link
Copy Markdown
Contributor

No description provided.

@seankane-msft

Copy link
Copy Markdown
Contributor Author

/azp run python - containerregistry - tests

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@seankane-msft

Copy link
Copy Markdown
Contributor Author

/azp run python - containerregistry - tests-weekly

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@seankane-msft

Copy link
Copy Markdown
Contributor Author

/azp run python - containerregistry - tests-weekly

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).


@acr_preparer()
def test_update_tag_properties(self, containerregistry_anonregistry_endpoint):
if not self.is_public_endpoint(containerregistry_anonregistry_endpoint):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can do a pytest.mark.skipif instead, even though you're looking at a self parameter

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get a NameError: name 'self' is not defined if I do this, also the parameter is passed in by the preparer so it wouldn't be available either.

endpoint = "https://" + endpoint
self._endpoint = endpoint
self.credential_scope = kwargs.get("authentication_scope", "https://management.core.windows.net/.default")
self.credential_scopes = kwargs.get("credential_scopes", ["https://management.core.windows.net/.default"])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be a private attr?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It probably should, but the ExchangeClient models are not exposed publicly

# 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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is credential_scopes an iterable (the name implies that it is)? If so, does the keyword paramtype need to be updated from str to something like List[str]?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it should be a List[str].

@swathipil swathipil left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@seankane-msft
Sean Kane (seankane-msft) merged commit 43b7b8c into Azure:main Jul 6, 2021
@seankane-msft
Sean Kane (seankane-msft) deleted the acr-credscope branch July 6, 2021 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants