Skip to content

Conversation

@pive
Copy link
Contributor

@pive pive commented Apr 5, 2021

Fixes #571 and allows more options for configuring s3.
Backward compatible and tests passed on Appveyor and TravisCI.

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]
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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:
Copy link
Contributor

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?

Copy link
Contributor Author

@pive pive Apr 5, 2021

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:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor

@springmeyer springmeyer left a 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.

@springmeyer springmeyer merged commit 26fac61 into mapbox:master Apr 5, 2021
@springmeyer
Copy link
Contributor

Tagged as @mapbox/node-pre-gyp@1.0.3

hyj1991 pushed a commit to X-Profiler/node-pre-gyp that referenced this pull request Jun 16, 2023
Allow alternative s3 urls and s3-compatible backends (e.g. minio).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

unable to publish to s3 - Cannot read property 'slice' of undefined

2 participants