-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add the ability to delete an etcd snapshot locally or from S3 #3277
Conversation
Signed-off-by: Brian Downs <[email protected]>
Signed-off-by: Brian Downs <[email protected]>
Signed-off-by: Brian Downs <[email protected]>
Signed-off-by: Brian Downs <[email protected]>
}, | ||
} | ||
|
||
func NewEtcdSnapshotCommand(action func(*cli.Context) error, subcommands []cli.Command) cli.Command { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer the old func NewEtcdSnapshotCommand(action func(*cli.Context) error)
signature. Use it to create the snapshot command and then modify the result by hanging the sub-command onto it. I think this would read cleaner in a PR and in general.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was the preferred route but doing so resulted in the binary being bloated passed the checked limit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
double-you-tee-eff-dot-gif
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we need to use the command factory pattern to bind different actions to the command and subcommands.
For the 'real' binaries we use the actual functions (Run, Delete, etc) as the command actions; for the self-extracting wrapper we need to bind the internalCLIAction to the command and subcommands so that it can (if necessary) extract out the real binaries and then exec them.
Signed-off-by: Brian Downs <[email protected]>
Signed-off-by: Brian Downs <[email protected]>
Signed-off-by: Brian Downs <[email protected]>
Signed-off-by: Brian Downs <[email protected]>
Signed-off-by: Brian Downs <[email protected]>
Signed-off-by: Brian Downs <[email protected]>
Signed-off-by: Brian Downs <[email protected]>
Signed-off-by: Brian Downs <[email protected]>
…#3277) * Add the ability to delete a given set of etcd snapshots from the CLI for locally stored and S3 store snapshots. Signed-off-by: Brian Downs <[email protected]>
…#3277) * Add the ability to delete a given set of etcd snapshots from the CLI for locally stored and S3 store snapshots. Signed-off-by: Brian Downs <[email protected]>
…#3277) * Add the ability to delete a given set of etcd snapshots from the CLI for locally stored and S3 store snapshots. Signed-off-by: Brian Downs <[email protected]>
…#3277) * Add the ability to delete a given set of etcd snapshots from the CLI for locally stored and S3 store snapshots. Signed-off-by: Brian Downs <[email protected]>
Proposed Changes
This change introduces the ability to perform a deletion of an etcd snapshot from either local storage or an S3 compatible object store.
A subcommand has been added to
etcd-snapshot
called "delete" that triggers this new behavior and takes all of the same flags as it's parent, allowing for the same default overrides and S3 access.The "delete" subcommand is depend upon positional arguments being the name of the snapshots to be removed. If none are provided, it errors out.
Types of Changes
This change will require a documentation update. I will do that when the rest of the etcd snapshot CLI CRUD operations are complete.
Verification
For locally stored etcd snapshots:
Create a snapshot
Get the name of the snapshot from the output.
For etcd snapshots stored in s3:
Create a snapshot
Get the name of the snapshot from the output.
Linked Issues
#3240
Further Comments