many: refactor in preparation for 'snap start' #3342

Merged
merged 6 commits into from May 23, 2017

Conversation

Projects
None yet
4 participants
Member

chipaca commented May 17, 2017

wrappers: move {Start,Stop}SnapServices(*snap.Info) to {Start,Stop}Services([]*snap.AppInfo)
snap/info: gave *snap.Info a Services() method which returns a []*snap.AppInfo of services
overlord/snapstate: propagate the refactor up to (but not including) the task level

@zyga zyga changed the title from this is a refactor branch, in preparation for 'snap start' etc to many: refactor in preparation for 'snap start' May 17, 2017

codecov-io commented May 18, 2017

Codecov Report

Merging #3342 into master will increase coverage by 0.11%.
The diff coverage is 71.79%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3342      +/-   ##
==========================================
+ Coverage   77.53%   77.65%   +0.11%     
==========================================
  Files         367      367              
  Lines       25156    25170      +14     
==========================================
+ Hits        19504    19545      +41     
+ Misses       3904     3880      -24     
+ Partials     1748     1745       -3
Impacted Files Coverage Δ
overlord/snapstate/backend/link.go 42.85% <0%> (ø) ⬆️
overlord/patch/patch5.go 6.66% <0%> (-0.23%) ⬇️
overlord/snapstate/aliasesv2.go 82.5% <100%> (ø) ⬆️
snap/info.go 91.06% <100%> (+19.29%) ⬆️
wrappers/binaries.go 60% <100%> (-4.71%) ⬇️
overlord/snapstate/handlers.go 69.95% <100%> (+0.21%) ⬆️
wrappers/services.go 79.56% <75%> (-2.92%) ⬇️
cmd/snap/cmd_aliases.go 94% <0%> (-2%) ⬇️
overlord/ifacestate/helpers.go 62.86% <0%> (ø) ⬆️
interfaces/sorting.go 96.66% <0%> (+3.33%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2380588...3ecf222. Read the comment docs.

overlord/snapstate/backend_test.go
f.ops = append(f.ops, fakeOp{
- op: "start-snap-services",
- name: info.MountDir(),
+ op: "start-snap-services",
@pedronis

pedronis May 18, 2017

Contributor

this is a bit "cheating", if one now replaces all the .Services() with nil tests would still pass. We probably want a couple of tests with services in snapstate now and then you can have the ifs, notice that the way to achieve that is to have a name like service-snap handled in backend_test.go in fake ReadInfo

@pedronis

pedronis May 18, 2017

Contributor

see alias-snap there for example

@pedronis

pedronis May 22, 2017

Contributor

now you can keep name: or something like that, svcs[0].Snap.MountDir()

overlord/snapstate/handlers.go
@@ -690,10 +690,15 @@ func (m *SnapManager) startSnapServices(t *state.Task, _ *tomb.Tomb) error {
if err != nil {
return err
}
+ svcs := currentInfo.Services()
+ // TODO:
@pedronis

pedronis May 18, 2017

Contributor

I fear is better to bite the bullet here given that you need to touch all the tests anyway.

@@ -280,6 +280,19 @@ func (s *Info) NeedsClassic() bool {
return s.Confinement == ClassicConfinement
}
+// Services returns a list of the apps that have "daemon" set.
+func (s *Info) Services() []*AppInfo {
@pedronis

pedronis May 18, 2017

Contributor

test in info_test.go ?

@@ -482,6 +495,11 @@ func (app *AppInfo) Env() []string {
return env
}
+// IsService returns whether app represents a daemon/service.
+func (app *AppInfo) IsService() bool {

chipaca added some commits May 17, 2017

many: refactor in preparation for 'snap start'
Moving things around to get ready for 'snap start' & etc. This means:
wrappers: move {Start,Stop}SnapServices(*snap.Info) to {Start,Stop}Services([]*snap.AppInfo)
snap/info: gave *snap.Info a Services() method which returns a []*snap.AppInfo of services
overlord/snapstate: propagate the refactor up to (but not including) the task level
wrappers,snap: move wrappers.IsService(app) to app.IsService()
Use app.IsService() instead of app.Daemon == "" all over the place.

thank you, lgtm

Very nice!

@@ -214,7 +213,7 @@ func refreshAliases(st *state.State, info *snap.Info, curAliases map[string]*Ali
newAliases = make(map[string]*AliasTarget, len(autoAliases))
// apply the current auto-aliases
for alias, target := range autoAliases {
- if app := info.Apps[target]; app == nil || wrappers.IsService(app) {
+ if app := info.Apps[target]; app == nil || app.IsService() {
@niemeyer

niemeyer May 23, 2017

Contributor

Nice!

- StartSnapServices(info *snap.Info, meter progress.Meter) error
- StopSnapServices(info *snap.Info, meter progress.Meter) error
+ StartServices(svcs []*snap.AppInfo, meter progress.Meter) error
+ StopServices(svcs []*snap.AppInfo, meter progress.Meter) error
@niemeyer

niemeyer May 23, 2017

Contributor

Double nice! :)

@chipaca chipaca merged commit 36a8b46 into snapcore:master May 23, 2017

7 checks passed

artful-amd64 autopkgtest finished (success)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
xenial-amd64 autopkgtest finished (success)
Details
xenial-i386 autopkgtest finished (success)
Details
xenial-ppc64el autopkgtest finished (success)
Details
yakkety-amd64 autopkgtest finished (success)
Details
zesty-amd64 autopkgtest finished (success)
Details

@chipaca chipaca deleted the chipaca:wrappers-services-start-stop-refactor branch May 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment