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

amazonka-s3: Rewrite requests to use vhost bucket endpoints #673

Merged
merged 2 commits into from
Oct 11, 2021

Conversation

endgame
Copy link
Collaborator

@endgame endgame commented Oct 11, 2021

Closes #630
Closes #628
Closes #610

@endgame
Copy link
Collaborator Author

endgame commented Oct 11, 2021

This is my first PR doing serious wrangling to requests, so I'd appreciate a close review.

@endgame endgame force-pushed the s3-vhost-endpoints branch 2 times, most recently from 72c4974 to 8d5087d Compare October 11, 2021 07:54
Copy link
Collaborator Author

@endgame endgame left a comment

Choose a reason for hiding this comment

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

I tested creating/destroying buckets in us-east-1 and eu-west-1, as well as GetBucketLocation and putting/getting small objects.

Comment on lines -58 to -61
"s3"
| virginia -> global "s3.amazonaws.com"
| china -> region ("s3." <> reg <> ".amazonaws.com.cn")
| s3 -> region ("s3-" <> reg <> ".amazonaws.com")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It appears that S3 now has a dotted regional endpoint for each region, so I removed this. I made test requests to s3.us-east-1.amazonaws.com.

"PutBucketTagging": ["Request.contentMD5Header"],
"PutObject": ["Request.expectHeader"]
"DeleteObjects": ["Request.contentMD5Header", "Request.s3vhost"],
"GetBucketLocation": [],
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

amazonka-s3/src/Network/AWS/S3/Internal.hs Show resolved Hide resolved
@endgame
Copy link
Collaborator Author

endgame commented Oct 11, 2021

@AriFordsham @RobertFischer Be great if you had a chance to test this PR too.

@endgame
Copy link
Collaborator Author

endgame commented Oct 11, 2021

Ping @K0Te - this may also fix your issue.

@endgame endgame added this to the 2.0 RC 1 milestone Oct 11, 2021
@AriFordsham
Copy link
Contributor

@endgame Thank you! I'm glad this vital package isn't bit-rotting.

I would love to test, but I would need to pull out some old code - I'll see what I can do.

@endgame
Copy link
Collaborator Author

endgame commented Oct 11, 2021

@AriFordsham In case it helps: https://github.com/gilt/kms-s3/pull/4 is an example PR where I modernised some amazonka usage. You'll need to change the revs in the cabal.project file, or do the stack equivalent.

The changelog also provides an overview of how library conventions have changed:

- Enums (sum types) have been replaced with pattern synonyms. (Thanks @rossabaker)
- This reduces GHC memory usage on some large packages (notably `amazonka-ec2`), as well as makes them more robust against new enum values in regions, responses, etc.
- Naming
- Record smart constructors (previously `describeInstances`, `getObject`, etc.) are now strictly prefixed with `new`, such as `newDescribeInstances`.
- Generated lenses are no longer exported from the top-level `Network.AWS.<name>` module - instead a `Network.AWS.<name>.Lens` module is provided.
- Generated lenses no longer use mnemonic or heuristically assigned prefixes such as `dirsrsInstances` and instead strictly prefix using the type name `describeInstances_instances` - following the form `<type>_<field>`.
- You may prefer to use a library like [`generic-lens`](https://hackage.haskell.org/package/generic-lens) instead of the long lens names.
- Exports
- Every `amazonka-*` package re-exports the `Network.AWS.Prelude` module.
- All type constructors (Such as record constructors) are now exported by default.

Copy link
Owner

@brendanhay brendanhay left a comment

Choose a reason for hiding this comment

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

LGTM! I'd rather avoid extra dependencies if possible, at least until everything has been pared back (looking at you, conduit) - but regex-posix is pretty lightweight so lets' ship it and worry about that later.

amazonka-s3/src/Network/AWS/S3/Internal.hs Show resolved Hide resolved
@endgame
Copy link
Collaborator Author

endgame commented Oct 11, 2021

I'd rather avoid extra dependencies if possible

Me too, but I chose to take on a regex package so the code would have a more obvious correspondence with the boto equivalent.

@endgame
Copy link
Collaborator Author

endgame commented Oct 11, 2021

@AriFordsham @RobertFischer @K0Te I'm merging this now but it would be good if you tested it.

@endgame endgame merged commit dc91c28 into brendanhay:develop Oct 11, 2021
@endgame endgame deleted the s3-vhost-endpoints branch October 11, 2021 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants