Make opts.hostedPath (and installation) respect the s3ForcePathStyle param
#608
Conversation
| @@ -33,6 +32,10 @@ module.exports.detect = function(opts, config) { | |||
| } | |||
| } | |||
| } | |||
| // If we are using s3ForcePathStyle the bucket is part of the http object path | |||
| // (but not the S3 key prefix path). If we aren't this isn't as much of a consideration. | |||
| const bucket_path = config.s3ForcePathStyle ? `/${config.bucket}/` : '/'; | |||
There was a problem hiding this comment.
Moving this below the if/else structure does skip an early return on line 21. I think that early return is acting in light of an error condition so not having config.prefix set for it is fine, but I'm not 100% sure.
|
Hi @jared-duo - thank you for this contribution. Looks totally reasonable to me. Have you confirmed/been able to test that it fixes the original issue? If so, can you think of a way to add a test to the unit tests to ensure this does not regress ever in the future? |
|
I have verified that this works for my use case. (Using the forked repo currently.) I'll TAL at the unit tests as soon as I can and see if I can come up with a reasonable way to test this - hopefully in the next week or so. Thanks for the feedback! |
|
#650 fixed #607. @jared-duo can you test and see if this PR is still needed or should we close it? |
This is an example of a potential fix for issue #607.
I'm sure I may be doing something non-idiomatically as I do not code for NodeJS too often. Also guessing that the CHANGELOG and package version will need incremented appropriately if this is actually something that
node-pre-gypwould like to do (and I'm not missing something.)