Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
hooks: support for install and remove hooks #3498
Conversation
stolowski
added some commits
Jun 13, 2017
All checks have failed
| + * | ||
| + */ | ||
| + | ||
| +package snaphooks |
pedronis
Jun 20, 2017
Contributor
given that this is not imported by snapstate it can probably exist as overlord/snapstate/hooks
| + | ||
| +prepare: | | ||
| + echo "Build basic test package" | ||
| + snapbuild $TESTSLIB/snaps/snap-install-hooks . |
zyga
Jun 20, 2017
Contributor
There's a better helper, install_local that caches built snaps and relieves you from the need to remove them below.
| + | ||
| + echo "Verify configuration value with snap get" | ||
| + snap get snap-install-hooks installed | MATCH 1 | ||
| + snap get snap-install-hooks foo | MATCH bar |
stolowski
Jun 20, 2017
Contributor
Good catch, I didn't commit 'configure' hook which does that (or rather, git ignored it silently due to my .gitignore ;)). Fixed now.
| + | ||
| + echo "Verify that remove hook is not executed when removing single revision" | ||
| + snap remove --revision=x1 snap-install-hooks | ||
| + if test -f $REMOVE_HOOK_FILE |
| @@ -0,0 +1 @@ | ||
| +#!/bin/sh |
fgimenez
Jun 20, 2017
Contributor
If the command is not going to be used we could declare it as command: "true" and remove bin/app
| @@ -0,0 +1 @@ | ||
| +#!/bin/sh |
| + snapbuild $TESTSLIB/snaps/snap-install-hooks-broken1 . | ||
| + | ||
| +restore: | | ||
| + rm snap-install-hooks_1.0_all.snap |
fgimenez
Jun 20, 2017
Contributor
No need to remove the snap files if the helper Zygmunt mentioned is used
| + snap remove --revision=x1 snap-install-hooks | ||
| + if test -f $REMOVE_HOOK_FILE | ||
| + then | ||
| + echo "Remove hook was executed. It shouldn't." |
fgimenez
Jun 20, 2017
Contributor
Indentation is broken (we are using 4 spaces), in each of the remainder if blocks too.
stolowski
added some commits
Jun 20, 2017
All checks have passed
codecov-io
commented
Jun 20, 2017
•
Edited 1 time
-
codecov-io
Jun 21, 2017
-
Jun 21, 2017
Codecov Report
@@ Coverage Diff @@
## master #3498 +/- ##
=========================================
Coverage ? 76.76%
=========================================
Files ? 379
Lines ? 26299
Branches ? 0
=========================================
Hits ? 20188
Misses ? 4324
Partials ? 1787
Continue to review full report at Codecov.
|
All checks have passed
stolowski
requested a review
from
niemeyer
Jun 22, 2017
niemeyer
requested changes
Jun 23, 2017
Thanks Pawel! This is pretty good, and the points below are perhaps a bit too nitpicky in some cases, so apologies in advance.
| @@ -102,6 +103,7 @@ func New() (*Overlord, error) { | ||
| o.hookMgr = hookMgr | ||
| o.stateEng.AddManager(o.hookMgr) | ||
| + hooks.SetupHooks(o.hookMgr) |
niemeyer
Jun 23, 2017
Contributor
Feels like a curious place to do this. The Register method is inside the hookMgr itself, so why would we expose SetupHooks just so something else can call into the package for it to register its own hooks on its own type? Can't we do this inside the Manager function itself, and not export the function?
stolowski
Jun 26, 2017
Contributor
Unfortunately we can't do it from snapstate's Manager (or I'm missing something?). Doing it from the Manager requires hookstate import and this creates import cycle (since hookstate imports snapstate).
niemeyer
Jun 27, 2017
Contributor
I don't understand the point. hookstate.Manager is within hookstate. It wouldn't have to import anything.
stolowski
Jun 28, 2017
Contributor
I see, I think I misunderstood your suggestion.
I moved hooks.go into hookstate package so hooks for snapstate are now registered from hookstate.Manager.
| + snapstate.RemoveHookSetup = RemoveHookSetup | ||
| +} | ||
| + | ||
| +func InstallHookSetup(st *state.State, snapName string) *state.Task { |
niemeyer
Jun 23, 2017
Contributor
This is reading like "Install the hook setup", which isn't what we mean here.
We can fix that by inverting: SetupInstallHook
Same for remove below.
| + Optional: true, | ||
| + } | ||
| + | ||
| + summary := fmt.Sprintf(i18n.G("Install hook of snap %q"), hooksup.Snap) |
niemeyer
Jun 23, 2017
Contributor
Similar problem here. We need the task summary to read like a task. Looking at the existing configure hook wording, this should be:
"Run install hook of %q snap"
Same for remove below.
| @@ -256,6 +263,14 @@ var Configure = func(st *state.State, snapName string, patch map[string]interfac | ||
| panic("internal error: snapstate.Configure is unset") | ||
| } | ||
| +var InstallHookSetup = func(st *state.State, snapName string) *state.Task { |
| + tasks = append(tasks, removeAliases, unlink, removeSecurity) | ||
| + addNext(state.NewTaskSet(tasks...)) | ||
| + } else { | ||
| + if removeHook != nil { |
| - ) | ||
| + "setup-aliases") | ||
| + if opts&install != 0 { | ||
| + expected = append(expected, "run-hook") |
niemeyer
Jun 23, 2017
Contributor
It's not enough to test it like that, because we don't know which hook is being run now that we can have many of them. We need to make sure that we have a proper install hooks on install, a proper remove hook on remove, and we also need to make sure that these are only added when first installing and last-removing the snap, and not otherwise. These checks should be done on the tests that verify that particular aspect (installation and removal), without adding custom logic to other tests like was done for the SkipConfigure one below.
stolowski
Jun 26, 2017
Contributor
Ok, I added checks to the generic TestInstallTasks / TestRemoveTasks and made sure these are the hooks we expect, plus added two new tests to make sure these hooks are not run when they shouldn't.
| @@ -5718,7 +5740,7 @@ func (s *snapmgrTestSuite) TestInstallMany(c *C) { | ||
| c.Check(installed, DeepEquals, []string{"one", "two"}) | ||
| for _, ts := range tts { | ||
| - verifyInstallUpdateTasks(c, 0, 0, ts, s.state) | ||
| + verifyInstallUpdateTasks(c, install, 0, ts, s.state) |
niemeyer
Jun 23, 2017
Contributor
This method is reading a bit awkward now. verifyInstallUpdate but really install. Let's please split them up:
- verifyInstallTasks
- verifyRefreshTasks
| for _, task := range ts.Tasks() { | ||
| - if task.Kind() == kind { | ||
| - return task | ||
| + if pred(task) { |
niemeyer
Jun 23, 2017
Contributor
Why moving the predicate out when there's only a single predicate we care about? Feels like early-generalization. The test looks simpler and more readable when we name this as tasksWithKind and don't ask for a predicate.
| + }) | ||
| + // only one hook task for install, no configure hook | ||
| + c.Assert(runHooks, HasLen, 1) | ||
| + c.Assert(runHooks[0].Summary(), Equals, `Install hook of snap "some-snap"`) |
niemeyer
Jun 23, 2017
Contributor
This doesn't look like the place to check for this. This test is about the checks below.
| @@ -0,0 +1,5 @@ | ||
| +name: snap-install-hooks-broken1 |
| @@ -0,0 +1,5 @@ | ||
| +name: snap-install-hooks |
stolowski
added some commits
Jun 26, 2017
All checks have passed
|
@niemeyer Ok, I think I've addressed all your comments, except for the one regarding SetupHook/Register, see my reply above. |
This was referenced Jun 26, 2017
| + | ||
| + echo "Verify that remove hook is executed" | ||
| + snap remove snap-hooks | ||
| + if ! test -f $REMOVE_HOOK_FILE; then |
kyrofa
Jun 26, 2017
•
Edited 1 time
-
kyrofa
Jun 26, 2017
Member
-
Jun 26, 2017
~/snap/snap-hooks/current is still valid, even after the snap is removed? That's not the case for /var/snap/snap-hooks/current, right? That seems a little odd.
stolowski
Jun 26, 2017
Contributor
Hmmm... That's interesting. Somehow ~/snap/snap-hooks/current gets removed in on my system in a couple of manual tests I've just done. But somehow the test doesn't fail. Something is fishy, I need to investigate with fresh mind. Thanks!
stolowski
Jun 27, 2017
Contributor
Ok, as discussed /root/snap/ is currently not cleaned on removal because we only clean "/home/*/snap/..." (it's probably a bug). I've changed the test to drop file in home directory instead so that the test doesn't rely on misbehavior that can soon change if we fix that problem.
| + | ||
| + echo "Forcing remove script to fail" | ||
| + snap set snap-hooks exitcode=1 | ||
| + snap remove snap-hooks |
kyrofa
Jun 26, 2017
Member
Will this test fail if this returns non-zero? Or should that be checked here?
stolowski
Jun 26, 2017
Contributor
No, it wouldn't fail on non-zero because of the setup of this hook has IgnoreError set. However, it's always 0 because of the problem with configure you spotted below - thanks! I've improved this test to make sure remove hook sees the correct exitcode value.
| + install_local snap-hooks | ||
| + | ||
| + echo "Forcing remove script to fail" | ||
| + snap set snap-hooks exitcode=1 |
stolowski
added some commits
Jun 26, 2017
Some checks were not successful
Some checks were not successful
Some checks were not successful
stolowski commentedJun 20, 2017
Add support for install and remove hooks. Install hook is executed only when snap is installed for the first time; it's run before configure hook and before services are started; remove hook is executed when last revision of snap gets removed.
Hooks for refresh (and revert) will be implemented in a separate PR.