-
-
Notifications
You must be signed in to change notification settings - Fork 609
feat(publisher-s3): add Cloudflare R2 provider #4125
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
base: main
Are you sure you want to change the base?
Conversation
|
|
||
| d(`executing: npx wrangler ${args.join(' ')}`); | ||
|
|
||
| return execa('npx', ['wrangler', ...args], { |
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.
It seems a bit heavy-handed to add execa as a dependency just to run an npx script. Since you're adding wrangler as a dependency anyways, does that library provide an equivalent JavaScript API that we could use?
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.
Hard agree. Would block this on the usage of npx, it's dangerous
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.
Yes! I didnt know but, as @MarshallOfSound said we could use the S3 endpoint, preventing the addition of wrangler and execa deps
I'll work on this
MarshallOfSound
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.
Doesn't R2 have an S3 compatible endpoint? We should just point the S3 publisher at that and avoid duplicating code and apparently shelling out to wrangler?
…instead of apiToken
…d secretAccessKey
…Token with accessKeyId and secretAccessKey
|
BTW I've published a package to test the implementation in my app and it's working 🫶 |
malept
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.
Does this have to be its own package? Can this functionality just be a set of configuration options for the existing publisher-s3 plugin, plus perhaps a guide on the docs site?
It should work well, since the API is like ~90% the same // Config would become:
{
name: '@electron-forge/publisher-s3',
config: {
provider: 'r2', // or 's3' (default)
// Common options
bucket: 'my-bucket',
accessKeyId: '...',
secretAccessKey: '...',
folder: 'releases',
region: "auto" // auto by default to r2
// R2-specific (only when provider: 'r2')
accountId: 'your-account-id',
// S3-specific (only when provider: 's3')
public: true,
sessionToken: '...',
s3ForcePathStyle: false,
releaseFileCacheControlMaxAge: 3600
}
} |
…s with detailed descriptions
…3 configuration details
erickzhao
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.
Hey @matos-ed, thanks for refactoring your PR. I'm unfamiliar with the interop between R2 and the AWS S3 SDK, so I went reading in the docs. I noticed this from the Cloudflare docs:
JavaScript or TypeScript users may continue to use the @aws-sdk/client-s3 ↗ npm package as per normal. You must pass in the R2 configuration credentials when instantiating your S3 service client.
https://developers.cloudflare.com/r2/examples/aws/aws-sdk-js-v3/
A lot of config options were marked as "S3 only", but I can't tell where the source of truth is for that. Could you help explain a bit further?
Hey @erickzhao const S3 = new S3Client({
region: "auto", // Required by SDK but not used by R2
endpoint: `https://${ACCOUNT_ID}.r2.cloudflarestorage.com`,
credentials: {
accessKeyId: ACCESS_KEY_ID,
secretAccessKey: SECRET_ACCESS_KEY,
},
});So in our Publisher-S3 integration to S3Client, if the R2 provider is selected, we:
let endpoint = this.config.endpoint;
if (provider === 'r2' && !endpoint) {
endpoint = `https://${this.config.accountId}.r2.cloudflarestorage.com`;
}
const region =
this.config.region || (provider === 'r2' ? 'auto' : undefined);
const s3Client = new S3Client({
credentials:
provider === 'r2'
? {
accessKeyId: this.config.accessKeyId!,
secretAccessKey: this.config.secretAccessKey!,
}
: this.generateCredentials(),
region,
endpoint,
forcePathStyle: !!this.config.s3ForcePathStyle,
});All of this params are currently supported by // R2-specific: Set ContentType
if (provider === 'r2') {
params.ContentType =
mime.lookup(artifact.path) || 'application/octet-stream';
}I've published it in |
Closes #4124
Add Cloudflare R2 Publisher
This PR adds support for publishing Electron Forge artifacts to Cloudflare R2 storage buckets.
What's New
Use Case
Provides an alternative to AWS S3 for hosting Electron app artifacts, offering potentially lower costs and better performance through Cloudflare's global network.
Configuration Example