-
Notifications
You must be signed in to change notification settings - Fork 175
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
Adding ByDiscardingBody() decorator. #118
Conversation
This issue is detailed in Azure#117
Question, would this func be included in all Responder funcs in the SDK, or just in the ones that do not use the response body? |
Just ones that don't use the body. Though, in theory it shouldn't hurt to add it everywhere. |
autorest/version.go
Outdated
@@ -7,7 +7,7 @@ import ( | |||
const ( | |||
major = "7" | |||
minor = "2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is not a patch, but a minor version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good point. :) I'll fix that up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't forget to also modify AutoRest!
@@ -21,6 +21,7 @@ and Responding. A typical pattern is: | |||
DoRetryForAttempts(5, time.Second)) | |||
|
|||
err = Respond(resp, | |||
ByDiscardingBody(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, formatting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this is in a code block, probably not a big deal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
yeah, code in markdown is funky, especially in PRs.
autorest/responder_test.go
Outdated
@@ -198,7 +202,8 @@ func TestByCopying_AcceptsNilBody(t *testing.T) { | |||
|
|||
func TestByClosing(t *testing.T) { | |||
r := mocks.NewResponse() | |||
err := Respond(r, ByClosing()) | |||
err := Respond(r, | |||
ByClosing()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this supposed to be ByDiscardingBody or was the line break intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had added ByDiscardingBody()
while on auto-pilot applying my fix, but thought better of it because this test should isolate ByClosing()
. I'll fix up this formatting so that it doesn't jump out to folks looking at diffs later.
* Adding ByDiscardingBody() decorator. This issue is detailed in Azure#117 * Updating reported version number. * Responding to PR feedback.
* Extract the authentication code in a separate package 'adal' * Update the log prefix to reflect the package path * Move the authorizer in its own file * Remove the dependency of service principal token on autorest package * Remove the dependency of device code flow on autorest package * Move the bearer authorizer in autorest package Introduce new interfaces for token provider and refresh functions. * Fix the example application * Fix the style of imports * Remove UTF-8 BOM from response body * Changelog (#114) * Added changelog for v7.2.4 * Ran glide up * Addimg BOM change * Correct version number * Adding ByDiscardingBody() decorator. (#118) * Adding ByDiscardingBody() decorator. This issue is detailed in #117 * Updating reported version number. * Responding to PR feedback. * Updating dependencies. * Adding ACR suffix to pulic cloud env (#104) * Adding ACR suffix to pulic cloud env (USgov, China and Germany still missing) * Added container registry DNS suffix in all envs * Added WithCustomBaseURL func (#116) * Added WithCustomBaseURL func * Bump version up * Fixing version test * Unfix version test * Updating change-log for v7.3.0 release. * Setting the current version to 8.0.0 * Add the ContentLength header in requests * Fix the build * Convert into a constant the Active Directory endpoint template * Read the body of HTTP response before checking its status * Use the status codes provided by Go HTTP library in device code test * Use the status code provide by Go HTTP library in the token test * Use the status codes provide by HTTP Go library in the authorization tests * Add a generic command line tool for adal library. This tool is based on the example from azure package. It is a bit more generic it the sense that one can retrieve a token for a given resource. * Add a README file for Azure Active Directory library for Go The documentation is based on the Readme file from azure/exmaple but the configuration steps were update for Azure CLI 2.0. In addition some code snippets for each authentication flow were added. * Update the error messages to use status codes provided by Go HTTP library
This issue is detailed in #117