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

Add a localPersister #1197

Merged
merged 5 commits into from
Jan 31, 2024
Merged

Add a localPersister #1197

merged 5 commits into from
Jan 31, 2024

Conversation

ankur22
Copy link
Collaborator

@ankur22 ankur22 commented Jan 29, 2024 •
edited
Loading

What?

This change adds a new LocalPersister which will replace how the screenshotter writes to the local disk.

Why?

This is the first step in removing the details of how and where the screenshot is being saved, allowing us to work with either a LocalPersister or (yet to be implemented) RemotePersister.

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: #1155, #1156 and #1157.

@ankur22 ankur22 marked this pull request as draft January 29, 2024 10:32
@ankur22 ankur22 mentioned this pull request Jan 29, 2024
Closed
3 tasks
@ankur22 ankur22 force-pushed the refactor/use-local-persister branch 4 times, most recently from 4e032de to b3ebc26 Compare January 29, 2024 15:58
@ankur22 ankur22 changed the base branch from main to add/local-file-persister January 29, 2024 15:59
@ankur22 ankur22 force-pushed the refactor/use-local-persister branch 2 times, most recently from 7377f20 to 7248c04 Compare January 29, 2024 16:04
@ankur22 ankur22 marked this pull request as ready for review January 29, 2024 16:06
@ankur22 ankur22 force-pushed the refactor/use-local-persister branch 3 times, most recently from 6fdb869 to ea3ff22 Compare January 29, 2024 16:52
@ankur22 ankur22 requested a review from inancgumus January 29, 2024 17:00
inancgumus
inancgumus reviewed Jan 30, 2024
View reviewed changes
common/screenshotter.go Outdated Show resolved Hide resolved
@ankur22 ankur22 mentioned this pull request Jan 30, 2024
Closed
3 tasks
@ankur22 ankur22 changed the base branch from add/local-file-persister to main January 30, 2024 11:24
@ankur22 ankur22 force-pushed the refactor/use-local-persister branch from ea3ff22 to c8d8cc7 Compare January 30, 2024 11:26
@ankur22 ankur22 changed the title Refactor to use localPersister Add a localPersister Jan 30, 2024
@ankur22 ankur22 force-pushed the refactor/use-local-persister branch 5 times, most recently from f161c6a to 3cc7c51 Compare January 30, 2024 11:51
inancgumus
inancgumus reviewed Jan 30, 2024
View reviewed changes
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.

LGTM in general 👍 Some suggestions.

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 Outdated Show resolved Hide resolved
storage/file_persister.go Outdated Show resolved Hide resolved
common/screenshotter.go Show resolved Hide resolved
common/page.go Outdated Show resolved Hide resolved
common/element_handle.go Outdated Show resolved Hide resolved
inancgumus
inancgumus previously approved these changes Jan 30, 2024
View reviewed changes
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.

🚀

inancgumus
inancgumus reviewed Jan 30, 2024
View reviewed changes
common/screenshotter.go Outdated Show resolved Hide resolved
common/screenshotter.go Outdated Show resolved Hide resolved
@ankur22 ankur22 force-pushed the refactor/use-local-persister branch from 70cd67a to c2f30b9 Compare January 31, 2024 09:37
@ankur22 ankur22 requested a review from inancgumus January 31, 2024 09:44
@ankur22
Add a local file persister
5c89130 
This is the file persister that will be used when testing locally. This
will replace the current implementation that is in screenshotter.
@ankur22
Add test for LocalFilePersister
ce03d7e
@ankur22
Add an instance of local persister
6936239 
Adding a localPersister will allow access to it from anywhere that has
access to the moduleVU. This change is a temporary refactor and will
need to be changed again once the remotePersister has been implemented
whereby either of the two will be chosen depending on the environment.
@ankur22
Refactor to work with the filePersister
2747713 
We're refactoring out the existing use of working directly with the os
package to persist screenshots to the local disk from screenshotter.
This change enables to work with the localPersister whereby the
underlying details of the where and how are hidden allowing us to
easily use either the localPersister (current) or the remotePersister
(future).
@ankur22
Remove the todo
ab79ae7 
This todo has been moved to the localPersister
@ankur22 ankur22 force-pushed the refactor/use-local-persister branch from c2f30b9 to ab79ae7 Compare January 31, 2024 09:44
@ankur22 ankur22 merged commit 396b149 into main Jan 31, 2024
17 checks passed
@ankur22 ankur22 deleted the refactor/use-local-persister branch January 31, 2024 10:18
This was referenced Jan 31, 2024
Closed
Closed
@BrewTestBot BrewTestBot mentioned this pull request Mar 25, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@inancgumus inancgumus inancgumus approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ankur22 @inancgumus