etcdctl: Split SaveWithVersion and client io#21248
etcdctl: Split SaveWithVersion and client io#21248sauerburger wants to merge 1 commit intoetcd-io:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sauerburger The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @sauerburger. Thanks for your PR. I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
| func SaveWithVersion(ctx context.Context, lg *zap.Logger, cfg clientv3.Config, dbPath string) (string, error) { | ||
| // SaveWithVersionStream fetches an etcd snapshot and writes it to the provided writer. | ||
| // It does NOT fsync or close the writer, which makes it safe for stdout and other streams. | ||
| func SaveWithVersionStream(ctx context.Context, lg *zap.Logger, cfg clientv3.Config, w io.Writer) (string, error) { |
There was a problem hiding this comment.
Recommend changing order of functions with SaveWithVersionStream after SaveWithVersion to make diff more readable.
| return &cobra.Command{ | ||
| Use: "save <filename>", | ||
| Short: "Stores an etcd node backend snapshot to a given file", | ||
| Use: "save [<filename>]", |
There was a problem hiding this comment.
There is too much risk of forgetting argument to result in terminal getting filled with binary gibberish.
Instead of making filename argument optional, it would be safer to - as indicator to write to stdout.
There was a problem hiding this comment.
Good point. Updated the PR accordingly.
sauerburger
force-pushed
the
main
branch
2 times, most recently
from
bfe3f32 to
bebac77
Compare
This commit splits the SaveWithVersion for snapshots into one part that takes care of opening, flushing, and closing the file while SaveWithVersionStream handles loading the snapshot and copying it to an io.Writer. This allows the CLI to call SaveWithVersionStream and write the snapshot to StdOut. Closes etcd-io#16242 Signed-off-by: Frank Sauerburger <frank@sauerburger.com>
3 participants
This commit splits the SaveWithVersion for snapshots into one part that takes care of opening, flushing, and closing the file while SaveWithVersionStream handles loading the snapshot and copying it to an io.Writer.
This allows the CLI to call SaveWithVersionStream and write the snapshot to StdOut.
Writing the snapshot to StdOut allows for secure, on-the-fly compression and encryption of the snapshot without first saving it to a filesystem.
Closes #16242
This is my first public MR in go. Feedback how to improve the MR highly welcome.