Skip to content

Conversation

@kryalama
Copy link
Contributor

@kryalama kryalama commented May 28, 2021

add support for sse redirect
add support for http proxy settings
use LazyAzureHttpClient in telemetry channel
Fix bug based on aad enabled flag

@kryalama kryalama requested a review from trask May 28, 2021 19:38
@kryalama kryalama requested review from heyams and xiao-lix as code owners May 28, 2021 19:38
@trask
Copy link
Member

trask commented May 28, 2021

also add v2.1/track for aad auth path and v2/track for non aad auth path (this is temporary fix until Breeze is stabilised)

Is this what other SDKs are doing?

@kryalama
Copy link
Contributor Author

kryalama commented Jun 1, 2021

also add v2.1/track for aad auth path and v2/track for non aad auth path (this is temporary fix until Breeze is stabilised)

Is this what other SDKs are doing?

synced with other sdks. If the SDK already supports Stamp Specific redirects(SSR), recommendation is to use v2.1/track. But for Python, SSR not yet supported , hence the implementation is different from Nodejs and Java.

final HttpRequest originalHttpRequest,
final int retryCount) {
// make sure the context is not modified during retry, except for the URL
context.setHttpRequest(originalHttpRequest.copy());
Copy link
Member

Choose a reason for hiding this comment

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

does this need to pass around originalHttpRequest or can it use context.getHttpRequest() each time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe we can use context.getHttpRequest, but I followed the same standard of setting the context from https://github.com/Azure/azure-sdk-for-java/blob/master/sdk/core/azure-core/src/main/java/com/azure/core/http/policy/RetryPolicy.java#L96

@lgtm-com
Copy link

lgtm-com bot commented Jun 7, 2021

This pull request introduces 1 alert when merging d777285 into 5837b5a - view on LGTM.com

new alerts:

  • 1 for Unused format argument

@kryalama kryalama requested a review from trask June 7, 2021 19:33
@kryalama kryalama changed the title add support for sse redirect add support for sse redirect and http proxy settings Jun 7, 2021
@lgtm-com
Copy link

lgtm-com bot commented Jun 8, 2021

This pull request introduces 1 alert when merging 453daac into 5837b5a - view on LGTM.com

new alerts:

  • 1 for Unused format argument

@lgtm-com
Copy link

lgtm-com bot commented Jun 14, 2021

This pull request introduces 2 alerts when merging d749135 into 4b730d0 - view on LGTM.com

new alerts:

  • 2 for Unused format argument

@lgtm-com
Copy link

lgtm-com bot commented Jun 15, 2021

This pull request introduces 2 alerts when merging a90132a into 9219bbf - view on LGTM.com

new alerts:

  • 2 for Unused format argument

@trask
Copy link
Member

trask commented Jun 15, 2021

  • 2 for Unused format argument

@kryalama when you have a chance, also check out the lgtm alert above

@kryalama kryalama requested a review from trask June 15, 2021 23:38
@trask trask merged commit 94ac07b into aad Jun 16, 2021
@trask trask deleted the kryalama/addsse branch June 16, 2021 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants