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

Added support for CloudflareR2 Storage adaptar #97

Closed
wants to merge 5 commits into from
Closed

Added support for CloudflareR2 Storage adaptar #97

wants to merge 5 commits into from

Conversation

Tushar98644
Copy link

@Tushar98644 Tushar98644 commented Oct 19, 2023

fixes appwrite/appwrite#3352

Screenshots :

Screenshot 2023-10-19 at 3 35 33 PM

@Tushar98644 Tushar98644 changed the title Added support for CloudfareR2 adaptar Added support for CloudfareR2 Storage adaptar Oct 19, 2023
@Tushar98644
Copy link
Author

Tushar98644 commented Oct 19, 2023

@stnguyen90 @gewenyu99 kindly review

@stnguyen90 stnguyen90 self-requested a review October 19, 2023 15:58
@bannu0snake
Copy link

hi @Tushar98644 , can you please let me know how did you setup php and testing env in your mac. any reference links ?

@Tushar98644
Copy link
Author

Tushar98644 commented Oct 20, 2023

hi @Tushar98644 , can you please let me know how did you setup php and testing env in your mac. any reference links ?

I just installed PHP and Composer,after installation you can add the project dependencies using composer install. You can have a look at the composer.json file , there you can find the scripts which used can be used to run the lining operations . For linting a file simply use composer format <file-path> and composer lint <file-path> to run the lining test

Copy link
Contributor

@stnguyen90 stnguyen90 left a comment

Choose a reason for hiding this comment

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

Great PR! 🤯 We left some comments during the review, please check them out.

Please also make sure to run the test locally and attach a screenshot of the result.

src/Storage/Device/CloudflareR2.php Show resolved Hide resolved
public function __construct(string $root, string $accessKey, string $secretKey, string $bucket, string $region = self::APAC, string $acl = self::ACL_PRIVATE)
{
parent::__construct($root, $accessKey, $secretKey, $bucket, $region, $acl);
$this->headers['host'] = $bucket.'.'.'s3'.'.'.$region.'.cloudflarestorage.com';
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have any documentation on the host?

Copy link

Choose a reason for hiding this comment

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

An S3 API URL looks like this:
https://accountid.r2.cloudflarestorage.com/bucketname

Where account id is your CF Account ID, like this:
https://905fa3fe223dcff4bf6b46911342bcfd.r2.cloudflarestorage.com/test

If you are using the EU Juristiction, you need to add .eu before the .r2, like this:
https://905fa3fe223dcff4bf6b46911342bcfd.eu.r2.cloudflarestorage.com/test

src/Storage/Device/CloudflareR2.php Outdated Show resolved Hide resolved
src/Storage/Storage.php Outdated Show resolved Hide resolved
tests/Storage/Device/CloudfareR2Test.php Outdated Show resolved Hide resolved
.github/workflows/tests.yml Show resolved Hide resolved
tests/Storage/Device/CloudfareR2Test.php Outdated Show resolved Hide resolved
@Tushar98644
Copy link
Author

Great PR! 🤯 We left some comments during the review, please check them out.

Please also make sure to run the test locally and attach a screenshot of the result.

Do you mean the linting tests? I have attached a screenshot for the same in the pr description

@Tushar98644 Tushar98644 changed the title Added support for CloudfareR2 Storage adaptar Added support for CloudflareR2 Storage adaptar Oct 26, 2023
@Tushar98644
Copy link
Author

@stnguyen90 any updates ?

Copy link
Contributor

@stnguyen90 stnguyen90 left a comment

Choose a reason for hiding this comment

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

Some minor changes.

Please also address https://github.com/utopia-php/storage/pull/97/files#r1370466673

Do you mean the linting tests? I have attached a screenshot for the same in the pr description

No, I mean actual tests. Something that confirms this code actually works with Cloudflare R2 storage.

src/Storage/Device/CloudflareR2.php Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
Tushar98644 and others added 2 commits October 31, 2023 21:40
Co-authored-by: Steven Nguyen <1477010+stnguyen90@users.noreply.github.com>
@Tushar98644
Copy link
Author

Some minor changes.

Please also address https://github.com/utopia-php/storage/pull/97/files#r1370466673

Do you mean the linting tests? I have attached a screenshot for the same in the pr description

No, I mean actual tests. Something that confirms this code actually works with Cloudflare R2 storage.

Screenshot 2023-10-31 at 9 44 56 PM

I am having some issues with the card details and adding the r2 storage subscription to my account , would really appreciate it if you could test it from your side

@gewenyu99
Copy link

Hey,

Due to time constraints, I'm going to mark this PR hacktoberfest-accepted for now so you get DO's Hacktoberfest rewards. We'll continue to work with you on this issue for review and merge.

When it is merged, we'll contact you for Appwrite-specific Hacktoberfest swag.

Thanks for helping us improve Appwrite!

@stnguyen90
Copy link
Contributor

I am having some issues with the card details and adding the r2 storage subscription to my account , would really appreciate it if you could test it from your side

How were you validating anything if you didn't have access to Cloudflare R2 storage?

Comment on lines +9 to +16
/**
* Regions constants
* Reference: https://developers.cloudflare.com/durable-objects/platform/data-location/
*/

/**
* Regions constants
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Please consolidate

@Tushar98644
Copy link
Author

I am having some issues with the card details and adding the r2 storage subscription to my account , would really appreciate it if you could test it from your side

How were you validating anything if you didn't have access to Cloudflare R2 storage?

I wasn't , i just followed the guide for adding a new adapter

@docimin
Copy link

docimin commented Nov 14, 2023

Any update?

@stnguyen90
Copy link
Contributor

Closing as this doesn't actually work.

@stnguyen90 stnguyen90 closed this Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🚀 Feature: Add support for Cloudflare R2 Storage
5 participants