-
Notifications
You must be signed in to change notification settings - Fork 89
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
fix LocationConstraint added for us-east-1 by the SDK #214
Conversation
actually use the patch
6b43f85
to
ae98669
Compare
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.
LGTM 🚀
✅ I tested it against LocalStack on macOS and can confirm it fixes the location constraint issue.
(there are still other issues with the demo app such as a ClassDefNotFoundException in the stepfunctions jar)
I guess it should not affect deployment against AWS but a quick test might be helpful.
@@ -1,6 +1,6 @@ | |||
{ | |||
"name": "serverless-localstack", | |||
"version": "1.0.4", | |||
"version": "1.0.5", |
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: package.lock
not updated
// copy params so that appending keys does not unintentioinallly | ||
// mutate params object argument passed in by user | ||
var copiedParams = AWS.util.copy(params); | ||
if (this.config.region !== 'us-east-1' && !params.CreateBucketConfiguration) { |
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.
nice catch 👍
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 PR adds a patch to the
aws-sdk
, using its own utilities to overwrite an S3 method.It fixes localstack/localstack-demo#52
In
aws-sdk/lib/services/s3.js
, there this createBucket method:However, the
if (hostname !== this.api.globalEndpoint && !params.CreateBucketConfiguration)
check to add theLocationConstraint
evaluate the endpoint (to see if it's a region specific one) against the global endpoint forus-east-1
.This patch changes the logic to simply check the region set in the config, and if different than
us-east-1
and a configuration is not set, sets it.I can now properly deploy the demo with the new S3 v2 provider which enforces the specific fact that you cannot set the
LocationConstraint
tous-east-1
.To reproduce, you can deploy https://github.com/localstack/localstack-demo/ with and without the fix, with
PROVIDER_OVERRIDE_S3=v2
when starting LocalStack.I however have no idea how to add a test for this.