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

[John Lenton]

Move removeClick to be a method of SnapPart. Get rid of some spurious packageYaml (and click manifest!?!) parsing this way.

[Sergio Schvezov (sergiusens)]

this looks great, I'm just nitpicking on the code comments (one is a carry over).

review: Needs Information

[lp:~chipaca/snappy/removeClick]

482. By John Lenton on 2015-05-29

Merged filepath into removeClick.

483. By John Lenton on 2015-06-08

Merged filepath into removeClick.

484. By John Lenton on 2015-06-08

improved "maybe" comment

[John Lenton (chipaca)]

Done.

[John Lenton (chipaca)]

Note the “maybe” comment goes away in unsetActiveClick anyway :)
(addressing this here conflicted...)

[Sergio Schvezov (sergiusens)]

review: Approve

=== modified file 'snappy/click.go'
--- snappy/click.go	2015-06-04 22:21:51 +0000
+++ snappy/click.go	2015-06-08 17:44:19 +0000
@@ -262,35 +262,6 @@
 	return manifest, err
 }
 
-func removeClick(clickDir string, inter interacter) (err error) {
-	m, err := parsePackageYamlFile(filepath.Join(clickDir, "meta", "package.yaml"))
-	if err != nil {
-		return err
-	}
-
-	if err := removeClickHooks(m, originFromBasedir(clickDir), false); err != nil {
-		return err
-	}
-
-	// maybe remove current symlink
-	currentSymlink := filepath.Join(filepath.Dir(clickDir), "current")
-	p, _ := filepath.EvalSymlinks(currentSymlink)
-	if clickDir == p {
-		if err := unsetActiveClick(p, false, inter); err != nil {
-			return err
-		}
-	}
-
-	err = os.RemoveAll(clickDir)
-	if err != nil {
-		return err
-	}
-
-	os.Remove(filepath.Dir(clickDir))
-
-	return nil
-}
-
 // generate the name
 func generateBinaryName(m *packageYaml, binary Binary) string {
 	var binName string

=== modified file 'snappy/click_test.go'
--- snappy/click_test.go	2015-06-03 14:03:45 +0000
+++ snappy/click_test.go	2015-06-08 17:44:19 +0000
@@ -357,7 +357,10 @@
 	_, err = os.Stat(instDir)
 	c.Assert(err, IsNil)
 
-	err = removeClick(instDir, nil)
+	yamlPath := filepath.Join(instDir, "meta", "package.yaml")
+	part, err := NewInstalledSnapPart(yamlPath, testOrigin)
+	c.Assert(err, IsNil)
+	err = part.remove(nil)
 	c.Assert(err, IsNil)
 
 	_, err = os.Stat(instDir)
@@ -429,7 +432,11 @@
 
 	s.buildFramework(c)
 	appdir := filepath.Join(s.tempdir, "apps", "hello", "1.0.1")
-	c.Assert(removeClick(appdir, nil), IsNil)
+	yamlPath := filepath.Join(appdir, "meta", "package.yaml")
+	part, err := NewInstalledSnapPart(yamlPath, testOrigin)
+	c.Assert(err, IsNil)
+	err = part.remove(nil)
+	c.Assert(err, IsNil)
 }
 
 func (s *SnapTestSuite) TestSnapRemovePackagePolicyWeirdClickManifest(c *C) {
@@ -444,7 +451,11 @@
 	manifestFile := filepath.Join(appdir, ".click", "info", "hello.manifest")
 	c.Assert(ioutil.WriteFile(manifestFile, []byte(`{"name": "xyzzy","type":"framework"}`), 0644), IsNil)
 
-	c.Assert(removeClick(appdir, nil), IsNil)
+	yamlPath := filepath.Join(appdir, "meta", "package.yaml")
+	part, err := NewInstalledSnapPart(yamlPath, testOrigin)
+	c.Assert(err, IsNil)
+	err = part.remove(nil)
+	c.Assert(err, IsNil)
 }
 
 func (s *SnapTestSuite) TestLocalOemSnapInstall(c *C) {
@@ -810,7 +821,10 @@
 
 	// and that it gets removed on remove
 	snapDir := filepath.Join(snapAppsDir, "foo.mvo", "1.0")
-	err = removeClick(snapDir, nil)
+	yamlPath := filepath.Join(snapDir, "meta", "package.yaml")
+	part, err := NewInstalledSnapPart(yamlPath, testOrigin)
+	c.Assert(err, IsNil)
+	err = part.remove(nil)
 	c.Assert(err, IsNil)
 	c.Assert(helpers.FileExists(binaryWrapper), Equals, false)
 	c.Assert(helpers.FileExists(snapDir), Equals, false)
@@ -866,7 +880,10 @@
 
 	// and that it gets removed on remove
 	snapDir := filepath.Join(snapAppsDir, "foo.mvo", "1.0")
-	err = removeClick(snapDir, new(progress.NullProgress))
+	yamlPath := filepath.Join(snapDir, "meta", "package.yaml")
+	part, err := NewInstalledSnapPart(yamlPath, testOrigin)
+	c.Assert(err, IsNil)
+	err = part.remove(&progress.NullProgress{})
 	c.Assert(err, IsNil)
 	c.Assert(helpers.FileExists(servicesFile), Equals, false)
 	c.Assert(helpers.FileExists(snapDir), Equals, false)

=== modified file 'snappy/purge_test.go'
--- snappy/purge_test.go	2015-06-02 20:46:07 +0000
+++ snappy/purge_test.go	2015-06-08 17:44:19 +0000
@@ -183,10 +183,15 @@
 func (s *purgeSuite) TestPurgeRemovedWorks(c *C) {
 	inter := &MockProgressMeter{}
 	pkgdir, ddir := s.mkpkg(c)
-	c.Assert(removeClick(pkgdir, inter), IsNil)
+
+	yamlPath := filepath.Join(pkgdir, "meta", "package.yaml")
+	part, err := NewInstalledSnapPart(yamlPath, testOrigin)
+	c.Assert(err, IsNil)
+	err = part.remove(inter)
+	c.Assert(err, IsNil)
 	c.Check(helpers.FileExists(ddir), Equals, true)
 
-	err := Purge("hello-app", 0, inter)
+	err = Purge("hello-app", 0, inter)
 	c.Check(err, IsNil)
 	c.Check(helpers.FileExists(ddir), Equals, false)
 }

=== modified file 'snappy/snapp.go'
--- snappy/snapp.go	2015-06-04 12:55:54 +0000
+++ snappy/snapp.go	2015-06-08 17:44:19 +0000
@@ -855,13 +855,41 @@
 		return ErrFrameworkInUse(deps)
 	}
 
-	if err := removeClick(s.basedir, pb); err != nil {
+	if err := s.remove(pb); err != nil {
 		return err
 	}
 
 	return RemoveAllHWAccess(QualifiedName(s))
 }
 
+func (s *SnapPart) remove(inter interacter) (err error) {
+	// TODO[JRL]: check the logic here. I'm not sure “remove
+	// everything if active, and the click hooks if not” makes
+	// sense. E.g. are we removing fmk bins on fmk upgrade? Etc.
+	if err := removeClickHooks(s.m, s.origin, false); err != nil {
+		return err
+	}
+
+	// remove "current" symlink if it points to this SnapPart
+	// (i.e. if this was the active version).
+	currentSymlink := filepath.Join(filepath.Dir(s.basedir), "current")
+	p, _ := filepath.EvalSymlinks(currentSymlink)
+	if s.basedir == p {
+		if err := unsetActiveClick(p, false, inter); err != nil {
+			return err
+		}
+	}
+
+	err = os.RemoveAll(s.basedir)
+	if err != nil {
+		return err
+	}
+
+	os.Remove(filepath.Dir(s.basedir))
+
+	return nil
+}
+
 // Config is used to to configure the snap
 func (s *SnapPart) Config(configuration []byte) (new string, err error) {
 	return snapConfig(s.basedir, s.origin, string(configuration))