pkg/ioutils: move atomic file-writers to a separate (pkg/atomicwriter) package #49171
Conversation
thaJeztah
added
status/2-code-review
impact/deprecation
kind/refactor
thaJeztah
force-pushed
the
move_atomicwriters
branch
2 times, most recently
from
154c285 to
9c2ae86
Compare
pkg/ioutils/fswriters_deprecated.go
Outdated
| func NewAtomicFileWriter(filename string, perm os.FileMode) (io.WriteCloser, error) { | ||
| return fswriter.NewAtomicFileWriter(filename, perm) | ||
| } | ||
|
|
||
| // AtomicWriteFile atomically writes data to a file named by filename and with the specified permission bits. | ||
| // NOTE: umask is not considered for the file's permissions. | ||
| // | ||
| // Deprecated: use [fswriter.AtomicWriteFile] instead. | ||
| func AtomicWriteFile(filename string, data []byte, perm os.FileMode) error { | ||
| return fswriter.AtomicWriteFile(filename, data, perm) | ||
| } |
There was a problem hiding this comment.
Could use some feedback / thoughts on this;
I was originally (some time ago) considering to move these to a atomic package, for a slightly nicer interface;
atomic.NewFileWriteror evenatomic.NewWriteratomic.WriteFileatomic.NewWriteSet
But I wasn't sure if that would be too easily confused / conflict with Go's atomic packages.
FWIW; there's some existing projects handling atomic write operations as well that we could even consider adopting;
- https://github.com/tailscale/tailscale/blob/ff095606ccff083160eb01a8a4cc062cacfe1a33/atomicfile/atomicfile.go
- https://github.com/natefinch/atomic
- (mostly Linux, not Windows); https://github.com/google/renameio
- (forked from k8s, but using a temp-directory, which may not work well in all cases if that dir is on a different filesystem) https://github.com/gomodules/atomic-writer
There was a problem hiding this comment.
Pushed a temporary second commit to use atomicwriter as name for this instead of fswriter - will squash if we think this looks good 👍
There was a problem hiding this comment.
atomic might indeed be confusing with the Go's atomic.
Other possible name I thought of was atomicfile, since this package mostly deals with files. Then WriteFile would become just Write and New would become NewWriter (it has less usages than WriteFile, so I don't feel bad about making it a bit longer 😄).
There was a problem hiding this comment.
Naming is hard!
I tried to keep WriteFile as a parallel to os.WriteFile as it's effectively a drop-in replacement, so I thought it would be nice to keep that name the same.
There was a problem hiding this comment.
atomicwriter sounds good to me! Seems like it's the best option to both:
- avoid confusion with
atomic/my LSP importing the wrong package and - keeping
WriteFilefor theosparallel`
thaJeztah
force-pushed
the
move_atomicwriters
branch
2 times, most recently
from
eb135d3 to
d774014
Compare
thaJeztah
changed the title
…) package Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah
force-pushed
the
move_atomicwriters
branch
from
d774014 to
7864454
Compare
|
I'll bring this one in, and update my CLI vendor to update uses of the old location 👍 |
/pkg#32989- What I did
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)