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

feat: S3Compatible storage adapter #76

Closed

Conversation

haymaker
Copy link

Description

The following PR creates a generic S3-Compatible storage adapter that extends the Utopia\Storage\Device\S3 class in order to provide for customized endpoints in vhost-style or path-style storage access.

Addressed Issue

This PR addresses #28 and attempts to do so using a least-intrusive method for code change. Scope was limited, as much as possible, to the implementation of the S3Compatible adapter. Other additions, modifications, or refactors that I may be able to address will have their own separate issues/PRs opened.

Change Details

Attempts were made to limit excess code modification. Several considerations had to be taken into account for this change:

  • Endpoint parameter should be configured as https://end.point without bucket suffix regardless of vhost/path styles. The S3Compatible class only supports https endpoints at the moment. This is a constraint related to the S3::call() method that would require more significant refactoring.
  • Vhost parameter is a simple boolean: true for vhost-style, false for path-style. Parameter brought down to S3 class and defaults to true in order to facilitate vhost vs. path styles for S3Compatible.
  • S3::listObjects() and S3::deletePath required modification to support vhost vs. path - there was no workaround for this.
  • tests\S3Base::testDeletePath() modified to support vhost vs. path.
  • Limitations due to current code (e.g. URI generation in S3::call() method) would need to be further addressed by other PR(s).

Tests

  • Unit tests available in Utopia\Tests\Storage\Device\S3CompatibleTest.
  • I tested against systems I have access to: MinIO, DigitalOcean Spaces, and Backblaze B2.
  • Tested path-style against MinIO, vhost-style against the others.
  • For S3CompatibleTest specifically, all tests returned OK (21 tests, 49 assertions).

@haymaker haymaker force-pushed the feat-28-s3compatible-storage-adapter branch from 0f908bf to 32f6d39 Compare March 23, 2023 10:05
@stnguyen90 stnguyen90 self-requested a review March 27, 2023 18:52
src/Storage/Device/S3.php Outdated Show resolved Hide resolved
src/Storage/Device/S3Compatible.php Outdated Show resolved Hide resolved
@haymaker
Copy link
Author

haymaker commented May 8, 2023

Thanks, will get these changes completed (and ensure latest main is merged into my branch).

This was my IDE - it's set to PSR-12 default for php8, I should have noticed this in the first place. I appreciate the heads up!

@haymaker haymaker force-pushed the feat-28-s3compatible-storage-adapter branch from 32f6d39 to 890b8c0 Compare June 5, 2023 17:56
@haymaker
Copy link
Author

haymaker commented Jun 5, 2023

Changes have been made, branch also rebased to latest upstream/main.

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.

@christyjacob4, what do you think about merging this in now and then we can iterate on this to maybe consolidate out other adapters?

@JoshiJoshiJoshi
Copy link

Any news?

@AndrewBucklin
Copy link

What’s the latest on this? Really hoping to use other S3-compatible storage platforms! 😊

Thank you!

@haymaker
Copy link
Author

@AndrewBucklin I'd forgotten this was still open, I thought it had been merged!

If this ends up needing to be rebased against main i can do it - it's been a while since I created this.

Depending on direction/use case, it could be used as a base for other S3-compatible storage adapter decorators.

@MBDOFF
Copy link

MBDOFF commented Nov 4, 2023

any news? @christyjacob4 @stnguyen90

@docimin
Copy link

docimin commented Nov 24, 2023

Hey @haymaker
Mind resolving the conflicts for us? Thanks!

@haymaker
Copy link
Author

Hey @haymaker Mind resolving the conflicts for us? Thanks!

Sure @docimin, I can get to it later today.

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.

Hmm I was hoping for a cleaner. bit these changes to S3 make it a little messy...I'm not sure if we want to move forward with this approach 😕

@stnguyen90
Copy link
Contributor

Sorry, we're going to opt for this instead: #103

@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants