search

kopeio / etcd-manager

operator for etcd: moved to https://github.com/kubernetes-sigs/etcdadm
Apache License 2.0
164 stars 45 forks source link

feat(etcd-manager-ctl): use backupname to delete backup instead of timestamp #352

Closed sudoforge closed 4 years ago

sudoforge commented 4 years ago 
This commit refactors the `runDeleteCommand` function of
`etcd-manager-ctl` to use the "backup name" instead of the "timestamp"
attribute. More often than not, users are more likely to become familiar
with the syntax of `etcd-manager-ctl restore-backup`, which uses the
`backupname` attribute to identify the target backup to restore.

By bringing the `delete-backup` subcommand in alignment with
`restore-backup`, we improve the user experience of an operator in the
process of restoring a cluster.
sudoforge commented 4 years ago

I ran into an issue the other day at $WORK, where two invalid commands were at the beginning of the command queue. I found the requirement to use the timestamp a little weird, when most other commands use the backup name.

Let me know if there are any changes you want here, folks. Thanks for all of your hard work!

justinsb commented 4 years ago

Thanks - this is a good idea. It's technically breaking, but as I agree it's so much more consistent and intuitive it seems reasonable.

If anyone objects, we could add a flag for deletion by timestamp, or just support matching either.

/approve /lgtm

sudoforge commented 4 years ago

Yeah, if we think adding a flag for deleting by timestamp is a good idea, feel free to comment here and I'll take care of it.

sudoforge commented 4 years ago

Thanks for merging, @justinsb!

justinsb commented 4 years ago

I'm happy to see whether people ask for the flag; if you actually want to add it we can certainly merge it, but otherwise it's fine to wait and see I think!

BTW, have you signed the kubernetes CLA? We're trying to merge this project into the kubernetes-sigs etcdadm ( https://github.com/kubernetes-sigs/etcdadm/pull/180#issuecomment-730408940 ), and you can see we're hitting some CLA issues. I don't know if it's your account or someone else's though... Have you signed the CLA (or are you able to sign the CLA?)

sudoforge commented 4 years ago

Done: https://github.com/kubernetes-sigs/etcdadm/pull/180#issuecomment-731619949

RE: the flag, I'm happy to wait and see if people ask for it.