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

gadget: allow content observer to have opinions about a change #9401

Merged

Conversation

bboozzoo
Copy link
Collaborator

Allow content observers to have opinions about a given change, with the ability
to request the original content to be preserved.

Allow content observers to have opinions about a given change, with the ability
to request the original content to be preserved.

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
@bboozzoo bboozzoo added Simple 😃 A small PR which can be reviewed quickly UC20 labels Sep 23, 2020
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
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.

some initial comments, mainly naming considerations

gadget/update.go Outdated

const (
ContentWrite ContentOperation = iota
ContentUpdate
ContentRollback

ChangeExecute ContentChangeDisposition = iota
Copy link
Collaborator

Choose a reason for hiding this comment

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

ChangeApply instead ?

gadget/update.go Outdated

const (
ContentWrite ContentOperation = iota
ContentUpdate
ContentRollback

ChangeExecute ContentChangeDisposition = iota
ChangeAbort
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this be the first value of the enum given that is used with errors to follow go patterns?

gadget/update.go Outdated
@@ -61,11 +61,16 @@ type ContentChange struct {
}

type ContentOperation int
type ContentChangeDisposition int
Copy link
Collaborator

Choose a reason for hiding this comment

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

we'll need a native speaker input here: should this be:

  • ...ChangeDisposition
  • ...ChangeResolution
  • ...ChangeDecision
  • ...ChangeAction

or yet something else?

@anonymouse64 or @degville any input on this?

the values of this can be like: apply, abort, preserve as seen below

Copy link
Member

Choose a reason for hiding this comment

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

Action sounds the most natural to me I think

Copy link
Contributor

Choose a reason for hiding this comment

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

Action sounds the most natural to me I think

Yes, I agree.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the input. ChangeAction it is then.

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
@pedronis pedronis self-requested a review September 23, 2020 16:07
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 but a question and small detail

gadget/update.go Outdated Show resolved Hide resolved
}

return false, nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

the intention expressed by false here (it was anyway not yet used) was wrong? will we even use the value of ContentChangeAction for rollback?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The use of false here was wrong, I tried to have the code return true for al the cases where it's fine to proceed with the change.

As for the rollback scenario, I don't see any need for inspecting the returned value at this point.

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
Copy link
Collaborator

@mvo5 mvo5 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, thank you!

@mvo5 mvo5 merged commit ff17d06 into snapcore:master Sep 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Simple 😃 A small PR which can be reviewed quickly
Projects
None yet
5 participants