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(s3): add getInstance to expose some utils #592

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

sandros94
Copy link

Missing getInstance for the S3 driver, this should allow to presign urls based on the existing driver options.

@sandros94 sandros94 requested a review from pi0 as a code owner February 12, 2025 20:09
@43081j
Copy link
Member

43081j commented Feb 19, 2025

can you add a test too?

similar to the one in the redis tests:

      it("exposes instance", () => {
        expect(driver.getInstance?.()).toBeInstanceOf(ioredis.default);
      });

@sandros94
Copy link
Author

I'm noticing that the can access directly with / separator test is failing on my end, but it is probably caused by the fact I'm using a mostly-compatible S3 I have available

Copy link

codecov bot commented Feb 21, 2025

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 60.05%. Comparing base (4d61c78) to head (a87d1f1).
Report is 169 commits behind head on main.

Files with missing lines Patch % Lines
src/drivers/s3.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #592      +/-   ##
==========================================
- Coverage   65.05%   60.05%   -5.01%     
==========================================
  Files          39       42       +3     
  Lines        4055     3655     -400     
  Branches      487      590     +103     
==========================================
- Hits         2638     2195     -443     
- Misses       1408     1457      +49     
+ Partials        9        3       -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pi0
Copy link
Member

pi0 commented Feb 21, 2025

Regarding tests, it seems good change here but it is not right that we always have potential to miss this. i think we should probably add a (required) option to testDriver to provide getinstance check. drivers don't can use () => true to bypass but more explicit. (idea for another PR)

@pi0 pi0 changed the title fix(s3): missing getInstance feat(s3): add getInstance to expose client Feb 21, 2025
@pi0
Copy link
Member

pi0 commented Feb 21, 2025

I'm also thinking if it is too early to expose a "client" off s3. If you check aws4fetch implementation, we mainly use it for signing. Many of other functionalities are internal.

Changing the return type after we expose it would be a breaking change also..

We could perhaps return an object like { getURL } to expose limited functionality but explicit interface so if later we change internal library it won't be a breaking change.

@pi0 pi0 marked this pull request as draft February 21, 2025 10:35
@pi0 pi0 changed the title feat(s3): add getInstance to expose client feat(s3): add getInstance to expose some utils Feb 21, 2025
@sandros94
Copy link
Author

Changing the return type after we expose it would be a breaking change also..

We could perhaps return an object like { getURL } to expose limited functionality but explicit interface so if later we change internal library it won't be a breaking change.

Oh, indeed.

I'll study this and propose a few potentially useful utils. One for sure being getUrl (for pre-signed urls) as it will be a useful one IMO

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.

3 participants