-
Notifications
You must be signed in to change notification settings - Fork 91
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: update installer to use https for download #78
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.
Thanks!
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, thanks for tackling this change!
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
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.
Please add this.
@AshanFernando can you check at this? |
@AshanFernando several projects are now falling apart. Please fix this. |
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.
merge please
@rehrumesh @mjzone |
@@ -3,7 +3,7 @@ | |||
var tar = require("tar"), | |||
zlib = require("zlib"), | |||
path = require("path"), | |||
http = require("http"), | |||
https = require("https"), |
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.
http = require('http'),
https = require('https'),
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.
keeps support for http as well as https
@@ -12,7 +12,7 @@ var download = function(downloadUrl, installPath, callback) { | |||
console.log( | |||
`Started downloading dynamodb-local from ${downloadUrl} into ${installPath}. Process may take few minutes.` | |||
); | |||
http | |||
https |
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.
const client = downloadUrl.toLowerCase().startsWith('https') ? https : http;
client
.get(downloadUrl, function (response) {
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.
Good idea @huseyin-caglayan-cko
Looks good! Please approve! |
merge please |
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.
merge, please?
Circling back here: I ditched this package (along with From there, I manually created needed tables inside the Docker image. To create tables in the Docker image, you append I'll update this comment once the problematic packages have been completely removed from my app. UPDATE: I removed the problematic packages from my app, and everything still works. I am officially done using these. Like I said, I suggest people do the same. |
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 fix worked for me.
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 worked for me.
I joined |
For those of you directly editing |
@DeeWBee I think the solution you are suggesting is a very complicated workaround and adding a new infra (docker) in project which may not be possible for all the projects. The "correct" fix is to patch the note: I say this is correct as this would work on local/docker/CI-CD environments with minimal changes but YMMV. If docker works for you then please continue doing that as you already tackled the most difficult part and made your project more robust. |
thanks for merging. any idea when this will be published to npm? |
if anyone needs this to work in your containers (for testing) until the package is updated, just throw those lines into your Dockerfile after installing your packages (
|
Or |
Helllo, @AshanFernando Can you publish this fix on npm too ? |
I wish a new released version to bump dependency of |
Please merge! |
the PR was merged, but it is still an old version in the npm package. |
This still doesn't work - was it released? |
looks like this needs a new npm release |
FYI: Despite this PR being merged, this fix to the package ( Solution: You can use aws-dynamodb-local, a maintained fork, instead. (Disclaimer: I am a contributor to this fork). It is a drop-in replacement for this package, and is updated to fix this bug. If you're using Migrating takes about 2 minutes, with a full guide in the README. Of course, it's all still open-source and MIT licensed. Ownership of this new package sits with a registered charity, that is committed to maintaining the package into the future and is open to contributions from the community. |
fix error with 403 on http://s3-us-west-2.amazonaws.com/dynamodb-local/dynamodb_local_latest.tar.gz