Skip to content
This repository has been archived by the owner on Jan 20, 2022. It is now read-only.

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

Merged
merged 2 commits into from
Nov 14, 2020
Merged

Conversation

sudoforge
Copy link
Contributor

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.

Benjamin Denhartog added 2 commits October 24, 2020 08:27
…mestamp

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
Copy link
Contributor Author

sudoforge commented Oct 28, 2020

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
Copy link
Contributor

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

@justinsb justinsb merged commit 50b4099 into kopeio:master Nov 14, 2020
@sudoforge
Copy link
Contributor Author

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
Copy link
Contributor Author

Thanks for merging, @justinsb!

@justinsb
Copy link
Contributor

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 ( kubernetes-retired/etcdadm#180 (comment) ), 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
Copy link
Contributor Author

Done: kubernetes-retired/etcdadm#180 (comment)

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants