tmpfs: add temporal overlay filesystem #27
Conversation
Codecov Report
@@ Coverage Diff @@
## master #27 +/- ##
==========================================
- Coverage 90.27% 88.43% -1.85%
==========================================
Files 6 7 +1
Lines 432 510 +78
==========================================
+ Hits 390 451 +61
- Misses 23 32 +9
- Partials 19 27 +8
Continue to review full report at Codecov.
|
utils.go
Outdated
| @@ -98,3 +98,31 @@ func WriteFile(fs Filesystem, filename string, data []byte, perm os.FileMode) er | |||
|
|
|||
| return err | |||
| } | |||
|
|
|||
| // CopyPath copies file across filesystems. | |||
There was a problem hiding this comment.
To be consistent with the rest of the utils, maybe this should be called CopyFile, although I would call it simply Copy.
There was a problem hiding this comment.
I think we decided not to provide a CopyFile (or CopyPath) in the public API, since it is tricky to provide consistent behavior across implementations (see this). So we'll limit ourselves to functions that exist in the standard library (e.g. os) for which we can provide sane behavior. So this would be private.
tmpfs/tmpfs.go
Outdated
| type tmpFs struct { | ||
| fs billy.Filesystem | ||
| tmp billy.Filesystem | ||
| tempFiles map[string]bool |
There was a problem hiding this comment.
make this attribute a map[string]billy.Filesystem, call it resolver, add add a resolve method instead of isTempFile. This should make the code even simpler I believe.
There was a problem hiding this comment.
That would be could, although I think we need to check explicitely if it's temp file or not to provide the functionality of copying it to fs in some cases, so it's not only about resolving Filsystem that should be accessed.
tmpfs/tmpfs.go
Outdated
| return t.fs.Rename(from, to) | ||
| } | ||
|
|
||
| func (t *tmpFs) Remove(path string) error { |
There was a problem hiding this comment.
should we remove the entry from the tmpFiles also? it is difficult to tell.
There was a problem hiding this comment.
This should be documented somewhere in the original API of billy.
There was a problem hiding this comment.
@alcortesm I think it's documented in the interface? In any case, it replicates os.Remove behavior.
👍 to remove it from tmpFiles map.
|
|
|
Let's call it |
|
@alcortesm The convention is that fs names end with |
|
On a second thought, yes, the router thing is quite misleading. |
|
Regarding naming, how about
Except for that - looks good to me. |
tmpoverlayfs/tmpfs.go
Outdated
| return err | ||
| } | ||
|
|
||
| return t.tmp.Remove(from) |
There was a problem hiding this comment.
remove entry from tmp files map?
|
@ajnavarro Clean up history if needed. |
- Added specific tests - Each method call to the exact method of the underlying filesystems - Added copyFile private logic
ajnavarro
force-pushed
the
feature/temp-overlay
branch
from
6587167 to
c8ef2d3
Compare
|
@smola done. |
No description provided.