-
Notifications
You must be signed in to change notification settings - Fork 267
Allow alternative s3 urls and s3-compatible backends (e.g. minio). #576
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
Conversation
README.md
Outdated
| - `host`: should be the root url of the storage server. | ||
| - `bucket` | ||
| - `region` | ||
| - `s3ForcePathStyle` [optional, default: `false`, must be set to `true` for minio servers] |
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.
Could you try to make this comment more generic? For example, there might be users that need to set this to true but are not using or familiar with minio: how would they know to set it to true / can you describe more detail about why it needs to be set to true?
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.
How about:
Set s3ForcePathStyle to true if the endpoint url should not be prefixed with the bucket name. If false (default), the server endpoint would be contructed as bucket_name.your_server.com.
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.
👍
README.md
Outdated
|
|
||
| Why then not require S3? Because while some applications using node-pre-gyp need to distribute binaries as large as 20-30 MB, others might have very small binaries and might wish to store them in a GitHub repo. This is not recommended, but if an author really wants to host in a non-S3 location then it should be possible. | ||
|
|
||
| It is also possible to use an s3-compatible storage backend, such as minio, as well as non-standard s3 endpoints. In this case, you can specify the following properties in the `binary` section: |
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.
Can you add a little more detail around what kind of errors or situations a user might hit such that they would need to provide these options?
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.
What about:
If you are not using a standard s3 path like bucket_name.s3(.-)region.amazonaws.com, you might get an error on publish because node-pre-gyp extracts the region and bucket from the host url. For example, you may have an on-premises s3-compatible storage server, or may have configured a specific dns redirecting to an s3 endpoint. In these cases, you can explicitely set the region and bucket properties to tell node-pre-gyp to use these values instead of guessing from the host property. The following values can be used in the binary section:
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.
👍
springmeyer
left a comment
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.
Looks great, thank you. Added a few inline questions asking for more doc details. But otherwise this looks good to merge and release in next release.
|
Tagged as @mapbox/node-pre-gyp@1.0.3 |
Allow alternative s3 urls and s3-compatible backends (e.g. minio).
Fixes #571 and allows more options for configuring s3.
Backward compatible and tests passed on Appveyor and TravisCI.