Skip to content

Conversation

@lmazuel
Copy link
Member

@lmazuel lmazuel commented Mar 17, 2021

Fix #16791

@ghost ghost added the Azure.Core label Mar 17, 2021
@lmazuel lmazuel requested review from chlowell and mccoyp March 17, 2021 17:22
"""
if _is_autorest_v3(client_class):
raise ValueError(
"Auth file or JSON dict are deprecated auth approach and are not supported anymore. "
Copy link
Member

Choose a reason for hiding this comment

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

This implies that these approaches are generally deprecated -- should the deprecation be specific to track 2 clients? e.g. by saying something like "azure-common factory methods are deprecated for client_class.__name__ and are not supported anymore."

Copy link
Member Author

Choose a reason for hiding this comment

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

It's under a "track2" if statement, is that not enough you think?

Copy link
Member

Choose a reason for hiding this comment

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

I think it should cover the cases where this should be raised, but I just wonder if users would be led to think that these factory methods wouldn't work for non-deprecated track 1 clients as well. Does that make sense?

Copy link
Member

Choose a reason for hiding this comment

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

This is assuming that most users wouldn't look at the source code to notice that the error is specific to this condition

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, I think we don't love auth file, and therefore being sligtly wide in the message is ok to me. But this is a good point to think.

@lmazuel lmazuel enabled auto-merge (squash) March 18, 2021 22:25
@lmazuel lmazuel disabled auto-merge March 18, 2021 22:48
@lmazuel
Copy link
Member Author

lmazuel commented Mar 18, 2021

Seems the storage failure is not related to this PR, forcing merge

@lmazuel lmazuel merged commit ef231f1 into master Mar 18, 2021
@lmazuel lmazuel deleted the auth_error branch March 18, 2021 22:49
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.

[Identity] Make azure-common file-based auth exceptions more helpful

5 participants