Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug fixes #11716

Merged
merged 4 commits into from
May 20, 2020
Merged

Bug fixes #11716

merged 4 commits into from
May 20, 2020

Conversation

AlexanderSher
Copy link
Contributor

…ual Studio fails when MFA required

- Fix Azure#11371: [BUG] AzureConfigurationBuilder fails to build connection strings when deployed to App Service
- Remove some dead code
…nto BugFixes

# Conflicts:
#	sdk/identity/Azure.Identity/src/InteractiveBrowserCredential.cs
#	sdk/identity/Azure.Identity/src/ManagedIdentityClient.cs
@AlexanderSher
Copy link
Contributor Author

#11371 is fixed in another PR

@@ -34,27 +36,26 @@ public AccessToken Succeeded(AccessToken token)
return token;
}

public AuthenticationFailedException FailAndWrap(Exception ex)
public Exception FailAndWrap(Exception ex)
Copy link
Member

Choose a reason for hiding this comment

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

Your not actually ever returning from this method. Perhaps it should have a void return type and be renamed to FailAndThrow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is that if FailAndWrap method doesn't return exception, compiler requires to return something from catch statement. So while method doesn't return anything, it is the easiest way for caller to tell that exception is thrown.

@@ -34,27 +36,26 @@ public AccessToken Succeeded(AccessToken token)
return token;
}

public AuthenticationFailedException FailAndWrap(Exception ex)
public Exception WrapAndThrow(Exception ex)
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps I'm being a bit pedantic here but I think the name should be FailAndThrow rather than WrapAndThrow. My reasoning behind this is that this is an instance method on CredentialDiagnosticScope and the high order bit is that we are "failing" the diagnostic scope and then we are throwing the exception (possibly wrapped). If you want to be most precise you could also call it FailWrapAndThrow but this might be a bit long winded.

Copy link
Contributor Author

@AlexanderSher AlexanderSher May 20, 2020

Choose a reason for hiding this comment

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

Changed to FailWrapAndThrow

@AlexanderSher AlexanderSher merged commit bff953f into Azure:master May 20, 2020
@AlexanderSher AlexanderSher deleted the BugFixes branch August 24, 2020 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants