Skip to content

Conversation

@xiangyan99
Copy link
Member

No description provided.

@ghost ghost added the Azure.Core label Apr 22, 2021
@xiangyan99 xiangyan99 marked this pull request as ready for review April 22, 2021 22:24
@jiasli
Copy link
Member

jiasli commented Apr 23, 2021

Fix #18225: Remove ARMPipelineClient._default_policies by utilizing per_call_policies from #17340

Comment on lines +49 to +50
if not config.http_logging_policy:
config.http_logging_policy = kwargs.get('http_logging_policy', ARMHttpLoggingPolicy(**kwargs))
Copy link
Member

Choose a reason for hiding this comment

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

These lines are unnecessary as the client will always have http_logging_policy configured:

self.http_logging_policy = kwargs.get('http_logging_policy') or ARMHttpLoggingPolicy(**kwargs)

Copy link
Member Author

Choose a reason for hiding this comment

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

We need it because in your test case, you use the azure.core.configure which does not honor http_logging_policy.

w/o it will fail the tests.

)
kwargs["policies"] = self._default_policies(**kwargs)
per_call_policies = kwargs.get('per_call_policies', [])
per_call_policies.append(ARMAutoResourceProviderRegistrationPolicy())
Copy link
Member

Choose a reason for hiding this comment

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

In azure-core, per_call_policies can be a list or object:

if isinstance(per_call_policies, Iterable):
for policy in per_call_policies:
policies.append(policy)
else:
policies.append(per_call_policies)

But here it can only be a list. Passing per_call_policies as an object causes failure in the append method.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch.

@xiangyan99 xiangyan99 enabled auto-merge (squash) April 23, 2021 18:12
@xiangyan99
Copy link
Member Author

/check-enforcer evaluate

@xiangyan99
Copy link
Member Author

/check-enforcer reset

@xiangyan99 xiangyan99 merged commit 227c700 into master Apr 26, 2021
@xiangyan99 xiangyan99 deleted the mgmt_core_update_pipeline branch April 26, 2021 02:14
Comment on lines +53 to +54
if isinstance(per_call_policies, Iterable):
per_call_policies.append(AsyncARMAutoResourceProviderRegistrationPolicy())
Copy link
Member

@jiasli jiasli Apr 26, 2021

Choose a reason for hiding this comment

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

append will fail if per_call_policies is a tuple, instead of a list:

AttributeError: 'tuple' object has no attribute 'append'

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.

4 participants