Skip to content
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

Replace clients host and scheme properties to cater to S3-Compatible backends #61

Merged
merged 8 commits into from
Aug 18, 2023

Conversation

oleiade
Copy link
Member

@oleiade oleiade commented Aug 16, 2023

What?

This Pull Request is massive, but it does a single thing: replace the existing AWSClient.host and AWSClient.scheme properties with a single endpoint property both on the AWSClient, and the AWSConfig.

This Pull Request adds an option to AWSConfig:

const awsConfig = new AWSConfig({
    region: __ENV.AWS_REGION,
    accessKeyId: __ENV.AWS_ACCESS_KEY_ID,
    secretAccessKey: __ENV.AWS_SECRET_ACCESS_KEY,
    sessionToken: __ENV.AWS_SESSION_TOKEN,
    endpoint: 'https://fra1.digitaloceanspaces.com'
})

In the ☝🏻 example, every request will now be sent to https://fra1.digitaloceanspaces.com instead of https://amazonaws.com.

Why?

This is an attempt to replicate the constructs in the node AWS SDK, as discussed in #57, and expose an endpoint property. Using this property, users can now indicate to the client classes instances via the AWSConfig.endpoint property or by setting it directly on the client the actual URL endpoint it should use to talk to AWS.

The reason for that is to cater to AWS-compatible services and stacks, such as in #21 #57 #50, and allow for a nicer integration of the SDK with services such as DigitalOcean spaces and Cloudflare R2 offer S3-compatible APIs or with Localstack, for instance.

This feature does not mean that we officially support alternative S3-compatible APIs, but it should offer the possibility to whoever needs to use it to do so at their own risk.

@oleiade oleiade self-assigned this Aug 16, 2023
@oleiade
Copy link
Member Author

oleiade commented Aug 16, 2023

Hi @Schachte, and @w0rd-driven 👋🏻

Just a heads-up that I believe this PR should help address your recent issues interoperating with Cloudflare R2 and Digital Ocean spaces.

Could you take it out for a spin, and let me know how it goes? 🙏🏻

Thank you! 🙇🏻

@oleiade oleiade added the enhancement New feature or request label Aug 16, 2023
@oleiade oleiade added this to the v0.10.0 milestone Aug 16, 2023
@Schachte
Copy link

Schachte commented Aug 16, 2023

Awesome work! I'll review this guy today. One callout without actually having dug too deep yet, was this comment:

#50 (comment)

endpoint definitely shouldn't have the scheme in it.

I can browse to see if that is problematic as the example above prefixes with https. I know the default scheme is assumed https, but wasn't sure if there were weird cases where it would end up like https://https:// etc.

Edit: Oh 🤦 , took a quick peek at your refactor and see endpoint is taking precedence 👍

Copy link

@Schachte Schachte left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for the fast follow-up. Left just a couple nits.

src/internal/config.ts Outdated Show resolved Hide resolved
src/internal/config.ts Outdated Show resolved Hide resolved
src/internal/endpoint.ts Show resolved Hide resolved
@oleiade oleiade merged commit 27e9dea into main Aug 18, 2023
@oleiade oleiade deleted the s3-support-other-backends branch August 18, 2023 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants