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

Add RemoteFilePersister #1202

Merged
merged 2 commits into from
Feb 1, 2024
Merged

Add RemoteFilePersister #1202

merged 2 commits into from
Feb 1, 2024

Conversation

ankur22
Copy link
Collaborator

@ankur22 ankur22 commented Jan 30, 2024

What?

This adds a remote persister, which will upload files (screenshots) to the remote location. It uses a two request mechanism to:

  1. Retrieve a presigned url for the given file with the name.
  2. Uploads the file with the retrieved presigned url.

Although it is possible to batch retrieve the pre-signed url, there is no easy way to retrieve the filenames that are required to retrieve the presigned urls, so it needs to be done one at a time when a request to persist the screenshot comes in.

Why?

To enable the browser module to upload screenshots to a remote location.

Checklist

  • I have performed a self-review of my code
  • I have added tests for my changes
  • I have commented on my code, particularly in hard-to-understand areas

Related PR(s)/Issue(s)

Updates: #1159

@ankur22 ankur22 marked this pull request as draft January 30, 2024 16:35
@ankur22 ankur22 force-pushed the refactor/use-local-persister branch 2 times, most recently from c2f30b9 to ab79ae7 Compare January 31, 2024 09:44
Base automatically changed from refactor/use-local-persister to main January 31, 2024 10:18
@ankur22 ankur22 force-pushed the add/remote-persister branch 3 times, most recently from e39604f to dfafb6c Compare January 31, 2024 11:11
@ankur22 ankur22 changed the title Add/remote persister Add RemoteFilePersister Jan 31, 2024
@ankur22 ankur22 marked this pull request as ready for review January 31, 2024 11:39
Copy link
Member

@inancgumus inancgumus left a comment

Choose a reason for hiding this comment

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

The feature is almost around the corner! Nice. Some suggestions.

storage/file_persister.go Outdated Show resolved Hide resolved
storage/file_persister.go Outdated Show resolved Hide resolved
storage/file_persister.go Outdated Show resolved Hide resolved
storage/file_persister.go Outdated Show resolved Hide resolved
storage/file_persister.go Outdated Show resolved Hide resolved
storage/file_persister_test.go Outdated Show resolved Hide resolved
storage/file_persister_test.go Outdated Show resolved Hide resolved
storage/file_persister_test.go Show resolved Hide resolved
storage/file_persister_test.go Outdated Show resolved Hide resolved
storage/file_persister.go Show resolved Hide resolved
@ankur22 ankur22 force-pushed the add/remote-persister branch 2 times, most recently from 0d716f1 to 678b100 Compare January 31, 2024 17:38
storage/file_persister.go Outdated Show resolved Hide resolved
storage/file_persister_test.go Outdated Show resolved Hide resolved
storage/file_persister_test.go Outdated Show resolved Hide resolved
storage/file_persister.go Outdated Show resolved Hide resolved
Copy link
Member

@inancgumus inancgumus left a comment

Choose a reason for hiding this comment

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

Nice work!

This will be used to upload files (screenshots) to a remote location.
It first retrieves the presigned url for the current file, then uploads
the file with the new presigned url.

It is possible to retrieve multiple presigned urls in one request, but
the issue is that we don't know the names of the files before hand
which is a requirement to retrieve the presigned urls.
The use of a default client might not be enough to cancel long running
http requests. Enforcing a 10 second timeout in the http client will
ensure that the request is timed out even if the context doesn't have a
timer/cancel attached to it.
@ankur22 ankur22 merged commit 12ab6e9 into main Feb 1, 2024
17 checks passed
@ankur22 ankur22 deleted the add/remote-persister branch February 1, 2024 13:51
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.

2 participants