Add local file persister #1195
Closed
Add local file persister #1195
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ankur22
force-pushed
the
add/local-file-persister
branch
2 times, most recently
from
3dc6b6f
to
b797637
Compare
This interface is what the local persister and remote persister will have to conform to. It will allow the browser module to easily abstract away the detail of where and how the file is being persisted.
The file persister will either be a local one or a remote one. The decision will be made when the moduleVU is created, and one will be initialised and set in the moduleVU instance.
ankur22
force-pushed
the
add/local-file-persister
branch
from
b797637
to
a1811ba
Compare
ankur22
force-pushed
the
add/local-file-persister
branch
4 times, most recently
from
f814d97
to
7a72b00
Compare
ankur22
force-pushed
the
add/local-file-persister
branch
from
7a72b00
to
991e511
Compare
inancgumus
reviewed
Jan 29, 2024
storage/file_persister.go
Outdated
| func write(w io.Writer, r io.Reader) error { | ||
| buf := make([]byte, 4096) | ||
| for { | ||
| n, err := r.Read(buf) | ||
| if n > 0 { | ||
| if _, writeErr := w.Write(buf[:n]); writeErr != nil { | ||
| return fmt.Errorf("writing to the local writer: %w", writeErr) | ||
| } | ||
| } | ||
| if err == io.EOF { | ||
| break | ||
| } | ||
| if err != nil { | ||
| return fmt.Errorf("reading from the reader: %w", err) | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Why don't we just use io.Copy here?
There was a problem hiding this comment.
I have no idea how i missed that 😆. Good suggestion, added to 7c39fb9
inancgumus
reviewed
Jan 29, 2024
storage/file_persister.go
Outdated
| } | ||
| defer func() { | ||
| if err := f.Close(); err != nil { | ||
| logger.Errorf("LocalFilePersister:Persist", "closing the local file %q: %v", cp, err) |
There was a problem hiding this comment.
I think we can handle this also from the call site if we return an error here. So we don't have to pass a logger here. The call site can log the error if it sees it's a file close error. WDYT?
There was a problem hiding this comment.
Yeah, that's a nice idea! Added it to 9476537
In which case the logger can be removed and has been 🎉
ankur22
force-pushed
the
add/local-file-persister
branch
from
cfa2726
to
80acf99
Compare
This is the file persister that will be used when testing locally. This will replace the current implementation that is in screenshotter.
ankur22
force-pushed
the
add/local-file-persister
branch
from
80acf99
to
0c350c1
Compare
Return an error to the caller if the close errors, but only if there isn't an existing error. This helps removes the logger that is no longer needed.
ankur22
force-pushed
the
add/local-file-persister
branch
from
b4b354e
to
9476537
Compare
ankur22
force-pushed
the
add/file-persister
branch
from
de5f6be
to
f35a27a
Compare
|
Closing this and moving this to #1197 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
This adds a local file persister which will save files to the local disk. It implements the interface defined in #1155.
Why?
So that we can replace how we're currently writing screenshots to disk. This will help hide the details of where and how the files are saved, allowing us to easily work with different file persisters.
Checklist
Related PR(s)/Issue(s)
Updates: #1156