-
Notifications
You must be signed in to change notification settings - Fork 77
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
Add support for custom roundtripper #140
Add support for custom roundtripper #140
Conversation
Signed-off-by: milinddethe15 <milinddethe15@gmail.com>
Signed-off-by: milinddethe15 <milinddethe15@gmail.com>
Signed-off-by: milinddethe15 <milinddethe15@gmail.com>
Signed-off-by: milinddethe15 <milinddethe15@gmail.com>
Signed-off-by: milinddethe15 <milinddethe15@gmail.com>
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.
Generally LGTM!
Just some minor nits
@@ -20,7 +23,7 @@ type TestCase struct { | |||
} | |||
|
|||
var validConfig = []byte(`storage_account: "myStorageAccount" | |||
storage_account_key: "abc123" | |||
storage_account_key: "bXlTdXBlclNlY3JldEtleTEyMyFAIw==" |
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.
Does this need to be base64 encoded?
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.
yes, otherwise it would give error.
Signed-off-by: milinddethe15 <milinddethe15@gmail.com>
@@ -201,7 +201,7 @@ func (s *overrideSignerType) Retrieve() (credentials.Value, error) { | |||
} | |||
|
|||
// NewBucketWithConfig returns a new Bucket using the provided s3 config values. | |||
func NewBucketWithConfig(logger log.Logger, config Config, component string) (*Bucket, error) { | |||
func NewBucketWithConfig(logger log.Logger, config Config, component string, rt http.RoundTripper) (*Bucket, error) { |
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 might be too late to this. But if we already have config.HTTPConfig.Transport
, why do we still introduce a new rt
parameter?
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.
We changed this in a follow up PR
Changes
Added a parameter in
NewBucket()
to pass custom roundtripperVerification
Created tests in which a
always error returning
roundtripper is passed to NewBucket() and tested whether error is caught or not.