-
Notifications
You must be signed in to change notification settings - Fork 150
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
feat(config,cache/s3proxy): add s3 addressing style support #662
feat(config,cache/s3proxy): add s3 addressing style support #662
Conversation
6202029
to
8c78612
Compare
/reopen to rerun the golang ci lint job |
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.
Thanks for the contribution. Sorry it took me a few days to look at this.
I fixed it. |
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.
This feature looks good to me, but I think we should use string flags instead, for improved readability.
I had to do some background reading to understand the use case. This blog post was useful, sharing it here for anyone reading through this PR later: https://aws.amazon.com/blogs/aws/amazon-s3-path-deprecation-plan-the-rest-of-the-story/
c743668
to
5fbf4c4
Compare
5fbf4c4
to
67dbda2
Compare
config/proxy.go
Outdated
// also when not found the type, return "auto" type. | ||
return valMap[typeStr] |
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 this function should probably check if typeStr
is in the map, and return an error if it isn't. And this error checking should be done in validateConfig()
in config/config.go
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 we only need to check it in validateConfig()
.
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.
Also I refactored it again.
49bc4de
to
dff949c
Compare
Thanks for following through with these changes- I made a few minor tweaks and landed this on the master branch. |
Some S3 providers do not support auto bucket lookup type, need to set it with
path
type ordns
type.Here I added a support to set it in
s3_proxy
filed in configuration to make it reachable.