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.

Sign up for GitHub

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

Jump to bottom

adds ceph adapter file and tests #88

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

adds ceph adapter file and tests #88

wants to merge 5 commits into from
+71 −2

Conversation

mugdha273
Copy link

Closes #6410
Here are the test results attached to it
image
image

@mugdha273
Copy link
Author

mugdha273 commented Oct 2, 2023 •
edited
Loading

Although I have a doubt that in Ceph.php in line 10 and 25,
How will I get region constants and '.$region.'.ceph.io'.
image

image

@gewenyu99
Copy link

👏 👏 👏 👏 👏 👏 👏 Thank you for including tests and results!

@stnguyen90 stnguyen90 self-requested a review October 12, 2023 15:44
stnguyen90
stnguyen90 requested changes Oct 12, 2023
View reviewed changes
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:

  1. to run the formatter to fix the lint errors
  2. attach a screenshot of successful tests

src/Storage/Device/Ceph.php Outdated
/**
* Regions constants
*/
const US_EAST_1 = 'us-east-1';
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there more regions than these?

Copy link
Author

Choose a reason for hiding this comment

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

I checked docs but couldn't find any specfied regions, I've added the most commonly accepted ones
const US_EAST_1 = 'us-east-1';

const US_EAST_2 = 'us-east-2';

const US_WEST_1 = 'us-west-1';

const US_WEST_2 = 'us-west-2';

I checked in blackblaze adpater and it clearly shows where it is hosted but it not the case with Ceph. I assume all regions would work here which present in S3.php file. Should I add all?

Copy link
Contributor

Choose a reason for hiding this comment

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

So they don't have any regions? Maybe we shouldn't add any in here then 🧐

src/Storage/Device/Ceph.php Outdated Show resolved Hide resolved
@Haimantika
Copy link

@mugdha273 please address the comments made by Steven.

@gewenyu99 gewenyu99 requested a review from stnguyen90 October 31, 2023 15:24
stnguyen90
stnguyen90 requested changes Oct 31, 2023
View reviewed changes
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.

Please see my latest comments. Please also attach a screenshot of successful tests

src/Storage/Device/Ceph.php Outdated
/**
* Regions constants
*/
const US_EAST_1 = 'us-east-1';
Copy link
Contributor

Choose a reason for hiding this comment

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

So they don't have any regions? Maybe we shouldn't add any in here then 🧐

src/Storage/Storage.php
@@ -83,7 +83,6 @@ public static function exists($name)
*
* Based on: https://stackoverflow.com/a/38659168/2299554
*
* @param int $bytes
Copy link
Contributor

Choose a reason for hiding this comment

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

why was this removed?

Copy link
Author

Choose a reason for hiding this comment

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

When I'm running composer lint these changes are automatically being made. Should I undo it? Because then the tests will fail right?

src/Storage/Storage.php Outdated
Comment on lines 39 to 40
* @param Device $device
* @return void
Copy link
Contributor

Choose a reason for hiding this comment

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

why were these removed?

Copy link
Author

Choose a reason for hiding this comment

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

When I'm running composer lint these changes are automatically being made. Should I undo it? Because then the tests will fail right?

@mugdha273
Copy link
Author

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

Please also make sure to:

  1. to run the formatter to fix the lint errors
  2. attach a screenshot of successful tests

image
In the main branch, if I run vendor/bin/phpunit --configuration phpunit.xml then also the tests are failing, so in my PR tests won't be able to pass well. Only formatters and lint tests can run

@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!

@github-actions github-actions bot mentioned this pull request Nov 1, 2023
Closed
@tessamero
Copy link

tessamero commented Nov 13, 2023 •
edited
Loading

Hello @mugdha273

Thank you for your contribution to Hacktoberfest 2023! We've noticed that your PR is still pending and requires some updates based on our engineering team's feedback.

We would love to see your PR successfully merged and send you the Appwrite swag as a token of appreciation. To remain eligible for the swag, please address the pending suggestions/make sure the tests are passing by Friday, November 17th. If the PR isn't updated by then, we will unfortunately have to close it due to the end of the Hacktoberfest event.

Looking forward to your updates and thank you!

@mugdha273
Copy link
Author

Hello @mugdha273

Thank you for your contribution to Hacktoberfest 2023! We've noticed that your PR is still pending and requires some updates based on our engineering team's feedback.

We would love to see your PR successfully merged and send you the Appwrite swag as a token of appreciation. To remain eligible for the swag, please address the pending suggestions/make sure the tests are passing by Friday, November 17th. If the PR isn't updated by then, we will unfortunately have to close it due to the end of the Hacktoberfest event.

Looking forward to your updates and thank you!

Hi my PR is complete though it is not passing tests. I fetched it from main branch and the tests are failing on main branch also, I've already asked my doubt, waiting for the reply.

@tessamero
Copy link

okay thanks @mugdha273 ill let the engineering team know you are awaiting a response

@gewenyu99
Copy link

Hey there! There were a lot of big PRs during this Hacktoberfest, and we wanted to give everyone ample time to collaborate with our engineering team. If you were able to merge your PRs during October, amazing. If it’s still not merged, don’t worry about it either. Either way, we’ve got your Hacktoberfest swag minted and ready to ship.

Please comment with your Discord username here so we can contact you about your shipping information to deliver your Hacktoberfest swag.

@mugdha273
Copy link
Author

mugdha273 commented Apr 10, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stnguyen90 stnguyen90 stnguyen90 requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
hacktoberfest-accepted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

🗄️ Extend Appwrite Storage with Ceph Adapter
5 participants
@mugdha273 @gewenyu99 @Haimantika @tessamero @stnguyen90