-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
feat: custom s3 endpoint url for 'plugin-node' #2176
feat: custom s3 endpoint url for 'plugin-node' #2176
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.
Hi @dtbuchholz! Welcome to the ai16z community. Thanks for submitting your first pull request; your efforts are helping us accelerate towards AGI. We'll review it shortly. You are now a ai16z contributor!
8f686e2
to
38f1a77
Compare
38f1a77
to
59068c4
Compare
Head branch was pushed to by a user without write access
59068c4
to
0f0e3f4
Compare
@odilitime thanks for the quick review! i just rebased on the latest from is there anything else I need to do on my end to get things merged? (I'm new to the repo and am just curious what the process looks like.) |
0f0e3f4
to
d5b4075
Compare
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Relates to
#2174, and builds upon #2379
Risks
Low. This affects only the
plugin-node
, and it changes logic in a way that is backward compatible.Background
What does this PR do?
This changes the
plugin-node
to, optionally, use the setAWS_S3_ENDPOINT
when generating URLs. If the value is set (e.g.,http://localhost:8014
), it instead uses this custom URL when reading/writing data. Closes #2174.What kind of change is this?
Feature; improves the S3 plugin functionality.
Why are we doing this? Any context or related work?
The S3 plugin lacked a small feature, which was still enforcing public AWS S3 URLs, even if you're using a local provider.
For reference, I'm building a web3 native object storage layer, which has S3 compatability—called Basin: https://basin.textile.io. We have an S3 adapter that works with MinIO, boto3, etc. I tested this plugin against our adapter, and everything worked as expected (more on this below).
Documentation changes needed?
N/A
Testing
Where should a reviewer start?
If you'd like to test this flow, you'll need to use some local S3 compatible server. A popular one is MinIO. Once you have the server running, it should log the local port. You can then set the
.env
variableAWS_S3_ENDPOINT
to this value (e.g.,http://localhost:8014
), and then calluploadFile
.Detailed testing steps
I tested this against our S3 adapter with both raw and resigned URLs. For example:
This assumes
AWS_S3_ENDPOINT
/AWS_FORCE_PATH_STYLE
are set.Discord username
dtb_