Merge lp:~sergiusens/snappy/inhibitSystemCtl into lp:~snappy-dev/snappy/snappy-moved-to-github

Proposed by Sergio Schvezov on 2015-04-06
Status: Rejected
Rejected by: Sergio Schvezov on 2015-04-08
Proposed branch: lp:~sergiusens/snappy/inhibitSystemCtl
Merge into: lp:~snappy-dev/snappy/snappy-moved-to-github
Diff against target: 49 lines (+8/-13)
2 files modified
snappy/click.go (+5/-7)
snappy/click_test.go (+3/-6)
To merge this branch: bzr merge lp:~sergiusens/snappy/inhibitSystemCtl
Reviewer Review Type Date Requested Status
Sergio Schvezov Disapprove on 2015-04-08
Michael Vogt 2015-04-06 Approve on 2015-04-08
Review via email: mp+255290@code.launchpad.net

Commit message

Inhibit systemctl completely on inhibithooks requests

Description of the change

The inhibit hooks flag was added for u-d-f, as such it is meant to use from clients on different systems; systemctl may not be available (it is not on the latest ubuntu lts), so we prefer to inhibit it completely and let the on boot hook run task (systemd unit) deal with the hook setup.

To post a comment you must log in.
Michael Vogt (mvo) wrote :

We probably need the first-boot script support for this to land first, right? Or has this landed already?

Systemctl enable is actually just setting a symlink into /etc/systemd/system/multi-user.target.wants to the service file - I wonder if we should just do that. The risk is of course that if its doing systemctl is doing more at some point we need to adjust. So first boot might be better (if we can get it done in a clean way).

review: Needs Information
Michael Vogt (mvo) wrote :

Another data point - deb-systemd-helper is implementing a "enable" too without the help of systemd (i.e. make_systemd_links and get_link_closure)

Michael Vogt (mvo) wrote :

I created lp:~mvo/snappy/snappy-systemctl-enable with my ideas for later. This branch is fine as is.

review: Approve
Snappy Tarmac (snappydevtarmac) wrote :

Attempt to merge into lp:snappy failed due to conflicts:

text conflict in snappy/click.go
text conflict in snappy/click_test.go

Sergio Schvezov (sergiusens) wrote :

Already in the systemd package rework.

review: Disapprove

Unmerged revisions

307. By Sergio Schvezov on 2015-04-06

Fixing tests:
- Check for len 0 for system hooks when inhibiting.
- Make it easier on editors that trim empty spaces.

306. By Sergio Schvezov on 2015-04-06

systemctl not to be called with inhibithooks

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'snappy/click.go'
2--- snappy/click.go 2015-04-01 09:20:35 +0000
3+++ snappy/click.go 2015-04-06 18:24:38 +0000
4@@ -506,13 +506,11 @@
5 if err := runSystemctl("daemon-reload"); err != nil {
6 return err
7 }
8- }
9-
10- if err := runSystemctl("enable", serviceName); err != nil {
11- return err
12- }
13-
14- if !inhibitHooks {
15+
16+ if err := runSystemctl("enable", serviceName); err != nil {
17+ return err
18+ }
19+
20 if err := runSystemctl("start", serviceName); err != nil {
21 return err
22 }
23
24=== modified file 'snappy/click_test.go'
25--- snappy/click_test.go 2015-04-01 03:11:44 +0000
26+++ snappy/click_test.go 2015-04-06 18:24:38 +0000
27@@ -449,10 +449,8 @@
28 // it go de-activated and finally 2.0 got activated
29 content, err := ioutil.ReadFile(filepath.Join(s.tempdir, "hook.trace"))
30 c.Assert(err, IsNil)
31- c.Assert(string(content), Equals, `now: ./bar_app_1.0.tracehook
32-now:
33-now: ./bar_app_2.0.tracehook
34-`)
35+ expected := fmt.Sprintf("now: ./bar_app_1.0.tracehook\nnow: \nnow: ./bar_app_2.0.tracehook\n")
36+ c.Assert(string(content), Equals, expected)
37 }
38
39 func (s *SnapTestSuite) TestClickCopyDataHookFails(c *C) {
40@@ -655,8 +653,7 @@
41 _, err := installClick(snapFile, InhibitHooks, nil)
42 c.Assert(err, IsNil)
43
44- c.Assert(allSystemctl[0], Equals, "enable")
45- c.Assert(allSystemctl, HasLen, 1)
46+ c.Assert(allSystemctl, HasLen, 0)
47 }
48
49 const expectedService = `[Unit]

Subscribers

People subscribed via source and target branches