-
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 replacing host from environment variable AWS_ENDPOINT_URL #238
Conversation
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.
Great work @jorgenfb 🥇
I tested the changes on macOS with the serverless.yml
from #237 and can confirm your PR fixes the issue.
I could also reproduce the initial issue, i.e., without this fix, AWS_ENDPOINT_URL=http://localhost:4566 sls deploy --stage dev
fails.
Thank you very much for this clean fix including a new regression test 🙏
@@ -784,7 +784,7 @@ class LocalstackPlugin { | |||
if (hostname && url.hostname === 'localhost') { | |||
url.hostname = hostname; | |||
} | |||
return url.href; | |||
return url.origin; |
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.
great catch 💯
@@ -177,7 +177,28 @@ describe("LocalstackPlugin", () => { | |||
expect(request.called).to.be.true; | |||
let templateUrl = request.firstCall.args[2].TemplateURL; | |||
// url should either start with 'http://localhost' or 'http://127.0.0.1 | |||
expect(templateUrl).to.satisfy((url) => url.startsWith(`${config.host}`) || url.startsWith('http://127.0.0.1')); | |||
expect(templateUrl).to.satisfy((url) => url === `${config.host}:4566/path/to/template` || url === 'http://127.0.0.1:4566/path/to/template'); |
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 refinement 👍
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.
Yeah, I wanted to make sure that this test also made sure the urls are correct after my changes.
expect(templateUrl).to.satisfy((url) => url === `${config.host}:4566/path/to/template` || url === 'http://127.0.0.1:4566/path/to/template'); | ||
}); | ||
|
||
it('should overwrite the S3 hostname with the value from environment variable', async () => { |
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.
kudos for adding this regression test case 👏 👏👏
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 jumping onto this issue, including analyzing the bug in the ticket, and providing a fix @jorgenfb 👏 🥳
I love that you also added a test! 🚀
Fixes #237