snap: load and validate implicit hooks. #1329

kyrofa · 2016-06-14T20:12:08Z

Implicit hooks are hooks present in the snap but not declared in the snap.yaml. This PR makes progress on LP: #1586465 by loading them along with an installed snap as well as a snap container.

Implicit hooks are hooks present in the snap but not declared in the snap.yaml. They are loaded along with an installed snap as well as a snap container. Updates: #1586465 Signed-off-by: Kyle Fazzari <kyle@canonical.com>

niemeyer · 2016-06-15T14:39:36Z

snap/container.go

+	ReadFile(relative string) ([]byte, error)
+
+	// ReadDir returns the content of a single directory inside the snap.
+	ReadDir(path string) ([]os.FileInfo, error)


Can we avoid os.FileInfo here? With that we force a pretty particular implementation of this method.

Sure, in my use-case I only need the file name, but changing this to []string limits the amount of information we get out of it, of course. Can you think of something else, or do you just want []string?

Would the implementations we can think of be reasonable if we had just ListDir(path string) []string?

Yeah I think so, I'll do that. It means they have to do a little more work to figure out if the children are files or directories, etc., but we don't have any requirements for directory walking right now.

I've been thinking some more about this. What if a snap author forgets to make the hook in question executable? Or has a directory in here instead? Wouldn't we want to know that when loading them info snap.Info and skip such hooks, or display a warning or error? Using []string here makes that difficult, as I mentioned before. What do you think?

mmh, we don't do those checks for apps either atm though, I agree that directory vs regular file is a bit borderline here though

@pedronis what do you think? Should we enrich this interface a little so we can get that type of data out of it?

Signed-off-by: Kyle Fazzari <kyle@canonical.com>

Also, return []string instead of []os.FileInfo. Signed-off-by: Kyle Fazzari <kyle@canonical.com>

niemeyer · 2016-06-15T19:30:36Z

LGTM

mvo5 · 2016-06-16T06:23:55Z

snap/implicit.go

+	// First of all, check to ensure the hooks directory exists. If it doesn't,
+	// it's not an error-- there's just nothing to do.
+	hooksDir := snapInfo.HooksDir()
+	if _, err := os.Stat(hooksDir); os.IsNotExist(err) {


You could use osutil.IsDirectory(hooksDir) here, but the os.Stat() is fine of course.

Ah, good idea.

mvo5 · 2016-06-16T06:45:16Z

Looks very nice overall, just some questions/suggestsions.

pedronis · 2016-06-16T08:35:07Z

snap/export_test.go

+	ImplicitSlotsForTests         = implicitSlots
+	ImplicitClassicSlotsForTests  = implicitClassicSlots
+	RelativeHooksDir              = relativeHooksDir
+	AddImplicitHooks              = addImplicitHooks


not sure there's a big win in term of testing of exposing these, instead of calling the Read* functions (that are actually what will be used and call them)

Agreed, done.

Signed-off-by: Kyle Fazzari <kyle@canonical.com>

pedronis · 2016-06-17T17:17:29Z

snap/squashfs/squashfs.go

+	}
+
+	dirPath = path.Clean(dirPath)
+	pattern := regexp.MustCompile(regexp.QuoteMeta(dirPath) + string(os.PathSeparator) + "?(.+)")


MustCompile is strange given that dirPath comes from outside, also I would add start/endline anchoring otherwise there may be strange corner cases, also if you exclude / ([^/] instead of .) from the tail end you don't need the check ContainsRune check?

Agreed, though I needed [^/\n\r]. More succinct indeed.

Signed-off-by: Kyle Fazzari <kyle@canonical.com>

pedronis · 2016-06-17T18:07:37Z

snap/squashfs/squashfs.go

+	}
+
+	dirPath = path.Clean(dirPath)
+	pattern, err := regexp.Compile("(?m)^/?" + regexp.QuoteMeta(dirPath) + string(os.PathSeparator) + "?([^/\r\n]+)$")


would like to avoid the ?s here if possible. What about --dest "_"

and prefixPath = path.Join("_", dirPath)

and in the regexp just do: "(?m)^" + QuoteMeta(prefixPath) + "/([^...]+)$"

Very good, done.

Signed-off-by: Kyle Fazzari <kyle@canonical.com>

pedronis · 2016-06-17T18:22:44Z

snap/squashfs/squashfs.go

+		return nil, err
+	}
+
+	prefixPath := filepath.Join("_", dirPath)


this can be path not filepath

Signed-off-by: Kyle Fazzari <kyle@canonical.com>

pedronis · 2016-06-17T18:24:06Z

snap/squashfs/squashfs.go

+	prefixPath := filepath.Join("_", dirPath)
+	pattern, err := regexp.Compile("(?m)^" + regexp.QuoteMeta(prefixPath) + "/([^/\r\n]+)$")
+	if err != nil {
+		return nil, err


would prefix this somehow: return nil, fmt.Errorf("internal error: cannot compile squashfs list dir regexp for %q: %v", dirPath, err) or something

Signed-off-by: Kyle Fazzari <kyle@canonical.com>

This was referenced Jun 15, 2016

many: support hook interfaces #1331

Closed

interfaces: use security tags to index security snippets. #1332

Merged

kyrofa force-pushed the feature/1586465/load_implicit_hooks branch from 22c3425 to 3c7720c Compare June 15, 2016 14:06

snap: load and validate implicit hooks.

9a71aa8

Implicit hooks are hooks present in the snap but not declared in the snap.yaml. They are loaded along with an installed snap as well as a snap container. Updates: #1586465 Signed-off-by: Kyle Fazzari <kyle@canonical.com>

kyrofa force-pushed the feature/1586465/load_implicit_hooks branch from 3c7720c to 9a71aa8 Compare June 15, 2016 14:15

niemeyer reviewed Jun 15, 2016
View reviewed changes

kyrofa added 2 commits June 15, 2016 15:13

snap: don't export addImplicitHooks* functions.

de97f17

Signed-off-by: Kyle Fazzari <kyle@canonical.com>

snap: rename container.ReadDir() to ListDir().

70106f3

Also, return []string instead of []os.FileInfo. Signed-off-by: Kyle Fazzari <kyle@canonical.com>

mvo5 reviewed Jun 16, 2016
View reviewed changes

pedronis reviewed Jun 16, 2016
View reviewed changes

snap: use osutil.IsDirectory

b332c16

Signed-off-by: Kyle Fazzari <kyle@canonical.com>

pedronis added the Reviewed label Jun 16, 2016

kyrofa and others added 3 commits June 17, 2016 14:15

snap: stop testing addImplicitHooks directly.

9338320

Signed-off-by: Kyle Fazzari <kyle@canonical.com>

squashfs: don't unpack a directory just to list its contents.

43ea197

Signed-off-by: Kyle Fazzari <kyle@canonical.com>

Merge branch 'master' into feature/1586465/load_implicit_hooks

7f33a4e

pedronis reviewed Jun 17, 2016
View reviewed changes

squashfs: use multiline match.

7be6afe

Signed-off-by: Kyle Fazzari <kyle@canonical.com>

snap: remove relativeHooksDir.

04b8fa6

Signed-off-by: Kyle Fazzari <kyle@canonical.com>

pedronis reviewed Jun 17, 2016
View reviewed changes

squashfs: clean up regex by using "_" as unsquashfs dest.

4adfc61

Signed-off-by: Kyle Fazzari <kyle@canonical.com>

pedronis reviewed Jun 17, 2016
View reviewed changes

squashfs: use path.Join instead of filepath.Join

9639415

Signed-off-by: Kyle Fazzari <kyle@canonical.com>

pedronis reviewed Jun 17, 2016
View reviewed changes

kyrofa added 2 commits June 17, 2016 18:29

squashfs: make invalid regex error more friendly.

3cb5bc7

Signed-off-by: Kyle Fazzari <kyle@canonical.com>

squashfs: go fmt.

e322fd9

Signed-off-by: Kyle Fazzari <kyle@canonical.com>

kyrofa merged commit 41dd8df into snapcore:master Jun 17, 2016

kyrofa deleted the feature/1586465/load_implicit_hooks branch June 17, 2016 19:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

snap: load and validate implicit hooks. #1329

snap: load and validate implicit hooks. #1329

kyrofa commented Jun 14, 2016 •

edited

niemeyer Jun 15, 2016

kyrofa Jun 15, 2016

niemeyer Jun 15, 2016

kyrofa Jun 15, 2016 •

edited

kyrofa Jun 15, 2016

kyrofa Jun 16, 2016

pedronis Jun 16, 2016

kyrofa Jun 17, 2016

niemeyer commented Jun 15, 2016

mvo5 Jun 16, 2016

kyrofa Jun 16, 2016

kyrofa Jun 16, 2016

mvo5 commented Jun 16, 2016

pedronis Jun 16, 2016

kyrofa Jun 17, 2016

pedronis Jun 17, 2016

kyrofa Jun 17, 2016 •

edited

pedronis Jun 17, 2016

kyrofa Jun 17, 2016

pedronis Jun 17, 2016

kyrofa Jun 17, 2016

pedronis Jun 17, 2016

kyrofa Jun 17, 2016

snap: load and validate implicit hooks. #1329

snap: load and validate implicit hooks. #1329

Conversation

kyrofa commented Jun 14, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyrofa Jun 15, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niemeyer commented Jun 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvo5 commented Jun 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyrofa Jun 17, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyrofa commented Jun 14, 2016 •

edited

kyrofa Jun 15, 2016 •

edited

kyrofa Jun 17, 2016 •

edited