Skip to content
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

cmd/snap, api, snapstate: implement "snap remove --purge" #6870

Merged
merged 12 commits into from
May 21, 2019

Conversation

stolowski
Copy link
Contributor

Implementation of snap remove --purge ... as proposed here: https://forum.snapcraft.io/t/automatic-snapshots-snap-remove-purge-proposal/11294/2 and discussed briefly a while ago.

The idea is to allow disabling automatic snapshot for given remove, without having to disable it globally for all snaps. This is useful for removing fat snaps (such as multipass) if the user doesn't want the backup.

client/snap_op.go Outdated Show resolved Hide resolved
cmd/snap/cmd_snap_op.go Outdated Show resolved Hide resolved
Copy link
Contributor

@chipaca chipaca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you.

@stolowski stolowski requested a review from pedronis May 15, 2019 15:25
Copy link
Contributor

@zyga zyga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a few suggestions an a question inline (though not related to the main feature).

cmd/snap/cmd_snap_op.go Outdated Show resolved Hide resolved
@@ -66,6 +66,10 @@ const (

var ErrNothingToDo = errors.New("nothing to do")

type RemoveFlags struct {
Purge bool
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please document this field. Could be as simple as refrain from creating data snapshot on snap remove

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mmh, should this go into flags.go instead? it's very far from Remove here anyway? cc @chipaca

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, yes, good catch. This is what Flags is for.

overlord/snapstate/snapstate.go Show resolved Hide resolved
tests/main/snapshot-basic/task.yaml Show resolved Hide resolved
Copy link
Collaborator

@pedronis pedronis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good overall but some nitpicks, on some wondering whether @chipaca has input

overlord/snapstate/snapstate.go Outdated Show resolved Hide resolved
@@ -991,6 +992,8 @@ func init() {
waitDescs.also(map[string]string{
// TRANSLATORS: This should not start with a lowercase letter.
"revision": i18n.G("Remove only the given revision"),
// TRANSLATORS: This should not start with a lowercase letter.
"purge": i18n.G("Remove the snap without creating snapshot data"),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Remove the snap without creating a snapshot of the data" ? cc @chipaca or s/of the/of its/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 to @pedronis' suggestion, Remove the snap without creating a snapshot of its data (although it's only a small thing).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed either of those reads better, although @degville would know more.

One thing though, would be nice if we used the save verb, as that's what we use for snapshots (ie something like Remove the snap without first saving a snapshot of its data)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(but the negation there feels weird to me)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about "Remove the snap without saving a snapshot of its data"? (note no "first")

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(but the negation there feels weird to me)

Yeah, it does a little. It's a little cleaner without first:
Remove the snap without saving a snapshot of its data

@@ -66,6 +66,10 @@ const (

var ErrNothingToDo = errors.New("nothing to do")

type RemoveFlags struct {
Purge bool
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mmh, should this go into flags.go instead? it's very far from Remove here anyway? cc @chipaca

overlord/snapstate/snapstate.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@pedronis pedronis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you

@mvo5 mvo5 merged commit 6118cf8 into canonical:master May 21, 2019
@robert-ancell
Copy link
Contributor

robert-ancell commented Aug 1, 2019

Supported in snapd-glib 1.50. REST API updated.

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

Successfully merging this pull request may close these issues.

7 participants