Merge lp:~chipaca/snappy/setActiveClick into lp:~snappy-dev/snappy/snappy-moved-to-github

[John Lenton]

moved setActivateClick to be a method of SnapPart

moved setActivateClick to be a SnapPart.activate.

And now, to share my pain, you get to watch https://www.youtube.com/watch?v=7_NL0SnpesM

[Sergio Schvezov (sergiusens)]

nice cleanup, since you put that ugly video in the description you get a needs fixing! :-P

review: Needs Fixing

[lp:~chipaca/snappy/setActiveClick]

483. By John Lenton on 2015-05-29

Merged removeClick into setActiveClick.

484. By John Lenton on 2015-06-08

Merged removeClick into setActiveClick.

485. By John Lenton on 2015-06-08

Merged removeClick into setActiveClick.

486. By John Lenton on 2015-06-08

addressed issues raised in review

[Sergio Schvezov (sergiusens)]

review: Approve

=== modified file 'snappy/click.go'
--- snappy/click.go	2015-06-08 18:57:10 +0000
+++ snappy/click.go	2015-06-08 18:57:10 +0000
@@ -866,70 +866,6 @@
 	return nil
 }
 
-func setActiveClick(baseDir string, inhibitHooks bool, inter interacter) error {
-	currentActiveSymlink := filepath.Join(baseDir, "..", "current")
-	currentActiveDir, _ := filepath.EvalSymlinks(currentActiveSymlink)
-
-	// already active, nothing to do
-	if baseDir == currentActiveDir {
-		return nil
-	}
-
-	// there is already an active part
-	if currentActiveDir != "" {
-		unsetActiveClick(currentActiveDir, inhibitHooks, inter)
-	}
-
-	// make new part active
-	newActiveManifest, err := readClickManifestFromClickDir(baseDir)
-	if err != nil {
-		return err
-	}
-
-	// yes, its confusing, we have two manifests, this is the important
-	// one, the YAML one
-	m, err := parsePackageYamlFile(filepath.Join(baseDir, "meta", "package.yaml"))
-	if err != nil {
-		return err
-	}
-
-	origin := originFromBasedir(baseDir)
-
-	if newActiveManifest.Type == pkg.TypeFramework {
-		if err := policy.Install(m.Name, baseDir); err != nil {
-			return err
-		}
-	}
-
-	if err := installClickHooks(baseDir, m, origin, inhibitHooks); err != nil {
-		// cleanup the failed hooks
-		removeClickHooks(m, origin, inhibitHooks)
-		return err
-	}
-
-	// generate the security policy from the package.yaml
-	if err := m.addSecurityPolicy(baseDir); err != nil {
-		return err
-	}
-
-	// add the "binaries:" from the package.yaml
-	if err := addPackageBinaries(baseDir); err != nil {
-		return err
-	}
-	// add the "services:" from the package.yaml
-	if err := addPackageServices(baseDir, inhibitHooks, inter); err != nil {
-		return err
-	}
-
-	// FIXME: we want to get rid of the current symlink
-	if err := os.Remove(currentActiveSymlink); err != nil && !os.IsNotExist(err) {
-		logger.Noticef("Failed to remove %q: %v", currentActiveSymlink, err)
-	}
-
-	// symlink is relative to parent dir
-	return os.Symlink(filepath.Base(baseDir), currentActiveSymlink)
-}
-
 // RunHooks will run all click system hooks
 func RunHooks() error {
 	systemHooks, err := systemClickHooks()

=== modified file 'snappy/click_test.go'
--- snappy/click_test.go	2015-06-08 18:57:10 +0000
+++ snappy/click_test.go	2015-06-08 18:57:10 +0000
@@ -299,7 +299,9 @@
 	pkgdir := filepath.Dir(filepath.Dir(yamlFile))
 	c.Assert(os.MkdirAll(filepath.Join(pkgdir, ".click", "info"), 0755), IsNil)
 	c.Assert(ioutil.WriteFile(filepath.Join(pkgdir, ".click", "info", "foox."+testOrigin+".manifest"), []byte(`{"name": "foox"}`), 0644), IsNil)
-	c.Assert(setActiveClick(pkgdir, true, ag), IsNil)
+	part, err := NewInstalledSnapPart(yamlFile, testOrigin)
+	c.Assert(err, IsNil)
+	c.Assert(part.activate(true, ag), IsNil)
 
 	pkg := makeTestSnapPackage(c, yaml+"version: 2")
 	_, err = installClick(pkg, 0, ag, testOrigin)
@@ -317,7 +319,9 @@
 	pkgdir := filepath.Dir(filepath.Dir(yamlFile))
 	c.Assert(os.MkdirAll(filepath.Join(pkgdir, ".click", "info"), 0755), IsNil)
 	c.Assert(ioutil.WriteFile(filepath.Join(pkgdir, ".click", "info", "foox."+testOrigin+".manifest"), []byte(`{"name": "foox"}`), 0644), IsNil)
-	c.Assert(setActiveClick(pkgdir, true, ag), IsNil)
+	part, err := NewInstalledSnapPart(yamlFile, testOrigin)
+	c.Assert(err, IsNil)
+	c.Assert(part.activate(true, ag), IsNil)
 
 	pkg := makeTestSnapPackage(c, yaml+"version: 2\nexplicit-license-agreement: Y")
 	_, err = installClick(pkg, 0, ag, testOrigin)
@@ -334,7 +338,9 @@
 	pkgdir := filepath.Dir(filepath.Dir(yamlFile))
 	c.Assert(os.MkdirAll(filepath.Join(pkgdir, ".click", "info"), 0755), IsNil)
 	c.Assert(ioutil.WriteFile(filepath.Join(pkgdir, ".click", "info", "foox."+testOrigin+".manifest"), []byte(`{"name": "foox"}`), 0644), IsNil)
-	c.Assert(setActiveClick(pkgdir, true, ag), IsNil)
+	part, err := NewInstalledSnapPart(yamlFile, testOrigin)
+	c.Assert(err, IsNil)
+	c.Assert(part.activate(true, ag), IsNil)
 
 	pkg := makeTestSnapPackage(c, yaml+"license-version: 3\nversion: 2")
 	_, err = installClick(pkg, 0, ag, testOrigin)
@@ -541,7 +547,7 @@
 	c.Assert(parts[1].IsActive(), Equals, true)
 
 	// set v1 active
-	err = setActiveClick(parts[0].(*SnapPart).basedir, false, nil)
+	err = parts[0].(*SnapPart).activate(false, nil)
 	parts, err = repo.Installed()
 	c.Assert(err, IsNil)
 	c.Assert(parts[0].Version(), Equals, "1.0")

=== modified file 'snappy/install_test.go'
--- snappy/install_test.go	2015-06-02 20:46:07 +0000
+++ snappy/install_test.go	2015-06-08 18:57:10 +0000
@@ -153,7 +153,9 @@
 	c.Assert(os.MkdirAll(filepath.Join(pkgdir, ".click", "info"), 0755), IsNil)
 	c.Assert(ioutil.WriteFile(filepath.Join(pkgdir, ".click", "info", "hello-app.manifest"), []byte(`{"name": "hello-app"}`), 0644), IsNil)
 	ag := &progress.NullProgress{}
-	c.Assert(setActiveClick(pkgdir, true, ag), IsNil)
+	part, err := NewInstalledSnapPart(yamlFile, "potato")
+	c.Assert(err, IsNil)
+	c.Assert(part.activate(true, ag), IsNil)
 	current := ActiveSnapByName("hello-app")
 	c.Assert(current, NotNil)
 

=== modified file 'snappy/parts_test.go'
--- snappy/parts_test.go	2015-06-02 20:46:07 +0000
+++ snappy/parts_test.go	2015-06-08 18:57:10 +0000
@@ -127,7 +127,10 @@
 	c.Assert(ioutil.WriteFile(filepath.Join(pkgdir, ".click", "info", "hello-app.manifest"), []byte(`{"name": "hello-app"}`), 0644), IsNil)
 	ag := &progress.NullProgress{}
 
-	c.Assert(setActiveClick(pkgdir, true, ag), IsNil)
+	part, err := NewInstalledSnapPart(yamlFile, testOrigin)
+	c.Assert(err, IsNil)
+
+	c.Assert(part.activate(true, ag), IsNil)
 
 	c.Check(PackageNameActive("hello-app"), Equals, true)
 	c.Assert(unsetActiveClick(pkgdir, true, ag), IsNil)

=== modified file 'snappy/purge.go'
--- snappy/purge.go	2015-05-20 17:24:29 +0000
+++ snappy/purge.go	2015-06-08 18:57:10 +0000
@@ -84,7 +84,7 @@
 		if pkg == nil {
 			continue
 		}
-		if err := setActiveClick(pkg.basedir, false, meter); err != nil {
+		if err := pkg.activate(false, meter); err != nil {
 			meter.Notify(fmt.Sprintf("Unable to activate %s: %s", pkg.Name(), err))
 		}
 	}

=== modified file 'snappy/purge_test.go'
--- snappy/purge_test.go	2015-06-08 18:57:10 +0000
+++ snappy/purge_test.go	2015-06-08 18:57:10 +0000
@@ -57,7 +57,7 @@
 	c.Check(inter.notified, HasLen, 0)
 }
 
-func (s *purgeSuite) mkpkg(c *C, args ...string) (string, string) {
+func (s *purgeSuite) mkpkg(c *C, args ...string) (dataDir string, part *SnapPart) {
 	version := "1.10"
 	extra := ""
 	switch len(args) {
@@ -78,19 +78,22 @@
 	c.Assert(os.MkdirAll(filepath.Join(pkgdir, ".click", "info"), 0755), IsNil)
 	c.Assert(ioutil.WriteFile(filepath.Join(pkgdir, ".click", "info", app+".manifest"), []byte(`{"name": "`+app+`"}`), 0644), IsNil)
 
-	ddir := filepath.Join(snapDataDir, app, version)
-	c.Assert(os.MkdirAll(ddir, 0755), IsNil)
-	canaryDataFile := filepath.Join(ddir, "canary.txt")
+	dataDir = filepath.Join(snapDataDir, app, version)
+	c.Assert(os.MkdirAll(dataDir, 0755), IsNil)
+	canaryDataFile := filepath.Join(dataDir, "canary.txt")
 	err = ioutil.WriteFile(canaryDataFile, []byte(""), 0644)
 	c.Assert(err, IsNil)
 
-	return pkgdir, ddir
+	part, err = NewInstalledSnapPart(yamlFile, testOrigin)
+	c.Assert(err, IsNil)
+
+	return dataDir, part
 }
 
 func (s *purgeSuite) TestPurgeActiveRaisesError(c *C) {
 	inter := &MockProgressMeter{}
-	pkgdir, _ := s.mkpkg(c)
-	c.Assert(setActiveClick(pkgdir, true, inter), IsNil)
+	_, part := s.mkpkg(c)
+	c.Assert(part.activate(true, inter), IsNil)
 
 	err := Purge("hello-app", 0, inter)
 	c.Check(err, Equals, ErrStillActive)
@@ -99,7 +102,7 @@
 
 func (s *purgeSuite) TestPurgeInactiveOK(c *C) {
 	inter := &MockProgressMeter{}
-	_, ddir := s.mkpkg(c)
+	ddir, _ := s.mkpkg(c)
 
 	err := Purge("hello-app", 0, inter)
 	c.Check(err, IsNil)
@@ -109,8 +112,8 @@
 
 func (s *purgeSuite) TestPurgeActiveExplicitOK(c *C) {
 	inter := &MockProgressMeter{}
-	pkgdir, ddir := s.mkpkg(c)
-	c.Assert(setActiveClick(pkgdir, true, inter), IsNil)
+	ddir, part := s.mkpkg(c)
+	c.Assert(part.activate(true, inter), IsNil)
 
 	err := Purge("hello-app", DoPurgeActive, inter)
 	c.Check(err, IsNil)
@@ -120,8 +123,8 @@
 
 func (s *purgeSuite) TestPurgeActiveRestartServices(c *C) {
 	inter := &MockProgressMeter{}
-	pkgdir, ddir := s.mkpkg(c, "v1", "services:\n - name: svc")
-	c.Assert(setActiveClick(pkgdir, true, inter), IsNil)
+	ddir, part := s.mkpkg(c, "v1", "services:\n - name: svc")
+	c.Assert(part.activate(true, inter), IsNil)
 
 	called := [][]string{}
 	systemd.SystemctlCmd = func(cmd ...string) ([]byte, error) {
@@ -143,8 +146,8 @@
 
 func (s *purgeSuite) TestPurgeMultiOK(c *C) {
 	inter := &MockProgressMeter{}
-	_, ddir0 := s.mkpkg(c, "v0")
-	_, ddir1 := s.mkpkg(c, "v1")
+	ddir0, _ := s.mkpkg(c, "v0")
+	ddir1, _ := s.mkpkg(c, "v1")
 
 	err := Purge("hello-app", 0, inter)
 	c.Check(err, IsNil)
@@ -155,9 +158,9 @@
 
 func (s *purgeSuite) TestPurgeMultiContinuesOnFail(c *C) {
 	inter := &MockProgressMeter{}
-	_, ddir0 := s.mkpkg(c, "v0")
-	_, ddir1 := s.mkpkg(c, "v1")
-	_, ddir2 := s.mkpkg(c, "v2")
+	ddir0, _ := s.mkpkg(c, "v0")
+	ddir1, _ := s.mkpkg(c, "v1")
+	ddir2, _ := s.mkpkg(c, "v2")
 
 	count := 0
 	anError := errors.New("fail")
@@ -182,12 +185,9 @@
 
 func (s *purgeSuite) TestPurgeRemovedWorks(c *C) {
 	inter := &MockProgressMeter{}
-	pkgdir, ddir := s.mkpkg(c)
+	ddir, part := s.mkpkg(c)
 
-	yamlPath := filepath.Join(pkgdir, "meta", "package.yaml")
-	part, err := NewInstalledSnapPart(yamlPath, testOrigin)
-	c.Assert(err, IsNil)
-	err = part.remove(inter)
+	err := part.remove(inter)
 	c.Assert(err, IsNil)
 	c.Check(helpers.FileExists(ddir), Equals, true)
 

=== modified file 'snappy/snapp.go'
--- snappy/snapp.go	2015-06-08 18:57:10 +0000
+++ snappy/snapp.go	2015-06-08 18:57:10 +0000
@@ -675,9 +675,16 @@
 	}
 
 	fullName := QualifiedName(s)
-	currentActiveDir, _ := filepath.EvalSymlinks(filepath.Join(s.basedir, "..", "current"))
 	dataDir := filepath.Join(snapDataDir, fullName, s.Version())
 
+	var oldPart *SnapPart
+	if currentActiveDir, _ := filepath.EvalSymlinks(filepath.Join(s.basedir, "..", "current")); currentActiveDir != "" {
+		oldPart, err = NewInstalledSnapPart(filepath.Join(currentActiveDir, "meta", "package.yaml"), s.origin)
+		if err != nil {
+			return "", err
+		}
+	}
+
 	if err := os.MkdirAll(s.basedir, 0755); err != nil {
 		logger.Noticef("Can not create %q: %v", s.basedir, err)
 		return "", err
@@ -720,17 +727,12 @@
 	// started then copy the data
 	//
 	// otherwise just create a empty data dir
-	if currentActiveDir != "" {
-		oldM, err := parsePackageYamlFile(filepath.Join(currentActiveDir, "meta", "package.yaml"))
-		if err != nil {
-			return "", err
-		}
-
+	if oldPart != nil {
 		// we need to stop making it active
-		err = unsetActiveClick(currentActiveDir, inhibitHooks, inter)
+		err = unsetActiveClick(oldPart.basedir, inhibitHooks, inter)
 		defer func() {
 			if err != nil {
-				if cerr := setActiveClick(currentActiveDir, inhibitHooks, inter); cerr != nil {
+				if cerr := oldPart.activate(inhibitHooks, inter); cerr != nil {
 					logger.Noticef("Setting old version back to active failed: %v", cerr)
 				}
 			}
@@ -739,7 +741,7 @@
 			return "", err
 		}
 
-		err = copySnapData(fullName, oldM.Version, s.Version())
+		err = copySnapData(fullName, oldPart.Version(), s.Version())
 	} else {
 		err = os.MkdirAll(dataDir, 0755)
 	}
@@ -757,10 +759,10 @@
 	}
 
 	// and finally make active
-	err = setActiveClick(s.basedir, inhibitHooks, inter)
+	err = s.activate(inhibitHooks, inter)
 	defer func() {
-		if err != nil && currentActiveDir != "" {
-			if cerr := setActiveClick(currentActiveDir, inhibitHooks, inter); cerr != nil {
+		if err != nil && oldPart != nil {
+			if cerr := oldPart.activate(inhibitHooks, inter); cerr != nil {
 				logger.Noticef("When setting old %s version back to active: %v", s.Name(), cerr)
 			}
 		}
@@ -803,7 +805,7 @@
 			}
 		}
 
-		if err := s.RefreshDependentsSecurity(currentActiveDir, inter); err != nil {
+		if err := s.RefreshDependentsSecurity(oldPart, inter); err != nil {
 			return "", err
 		}
 
@@ -831,7 +833,55 @@
 
 // SetActive sets the snap active
 func (s *SnapPart) SetActive(pb progress.Meter) (err error) {
-	return setActiveClick(s.basedir, false, pb)
+	return s.activate(false, pb)
+}
+
+func (s *SnapPart) activate(inhibitHooks bool, inter interacter) error {
+	currentActiveSymlink := filepath.Join(s.basedir, "..", "current")
+	currentActiveDir, _ := filepath.EvalSymlinks(currentActiveSymlink)
+
+	// already active, nothing to do
+	if s.basedir == currentActiveDir {
+		return nil
+	}
+
+	// there is already an active part
+	if currentActiveDir != "" {
+		unsetActiveClick(currentActiveDir, inhibitHooks, inter)
+	}
+
+	if s.Type() == pkg.TypeFramework {
+		if err := policy.Install(s.Name(), s.basedir); err != nil {
+			return err
+		}
+	}
+
+	if err := installClickHooks(s.basedir, s.m, s.origin, inhibitHooks); err != nil {
+		// cleanup the failed hooks
+		removeClickHooks(s.m, s.origin, inhibitHooks)
+		return err
+	}
+
+	// generate the security policy from the package.yaml
+	if err := s.m.addSecurityPolicy(s.basedir); err != nil {
+		return err
+	}
+
+	// add the "binaries:" from the package.yaml
+	if err := addPackageBinaries(s.basedir); err != nil {
+		return err
+	}
+	// add the "services:" from the package.yaml
+	if err := addPackageServices(s.basedir, inhibitHooks, inter); err != nil {
+		return err
+	}
+
+	if err := os.Remove(currentActiveSymlink); err != nil && !os.IsNotExist(err) {
+		logger.Noticef("Failed to remove %q: %v", currentActiveSymlink, err)
+	}
+
+	// symlink is relative to parent dir
+	return os.Symlink(filepath.Base(s.basedir), currentActiveSymlink)
 }
 
 // Uninstall remove the snap from the system
@@ -1035,7 +1085,11 @@
 }
 
 // RefreshDependentsSecurity refreshes the security policies of dependent snaps
-func (s *SnapPart) RefreshDependentsSecurity(oldBaseDir string, inter interacter) (err error) {
+func (s *SnapPart) RefreshDependentsSecurity(oldPart *SnapPart, inter interacter) (err error) {
+	oldBaseDir := ""
+	if oldPart != nil {
+		oldBaseDir = oldPart.basedir
+	}
 	upPol, upTpl := policy.AppArmorDelta(oldBaseDir, s.basedir, s.Name()+"_")
 
 	deps, err := s.Dependents()

=== modified file 'snappy/snapp_test.go'
--- snappy/snapp_test.go	2015-06-04 13:34:57 +0000
+++ snappy/snapp_test.go	2015-06-08 18:57:10 +0000
@@ -1158,7 +1158,7 @@
 	pb := &MockProgressMeter{}
 	m, err := parsePackageYamlData([]byte(yaml))
 	part := &SnapPart{m: m, origin: testOrigin, basedir: d1}
-	c.Assert(part.RefreshDependentsSecurity(d2, pb), IsNil)
+	c.Assert(part.RefreshDependentsSecurity(&SnapPart{basedir: d2}, pb), IsNil)
 	c.Check(touched, DeepEquals, []string{fn})
 }