many: modify snap run to understand component hooks #13976

andrewphelpsj · 2024-05-17T08:50:21Z

This PR enables the "snap run" command to run component hooks. It modifies the meaning of the "--hook" parameter a bit to allow snapd to run component hooks like so:

snap run --hook install snapname+compname

codecov-commenter · 2024-06-13T15:29:07Z

Codecov Report

Attention: Patch coverage is 78.61915% with 96 lines in your changes missing coverage. Please review.

Project coverage is 78.71%. Comparing base (ab996a0) to head (4e1002a).
Report is 336 commits behind head on master.

Files	Patch %	Lines
interfaces/repo.go	50.00%	10 Missing and 6 partials ⚠️
snap/snaptest/snaptest.go	0.00%	15 Missing ⚠️
cmd/snap/cmd_run.go	85.56%	9 Missing and 5 partials ⚠️
interfaces/ifacetest/app_set.go	58.82%	12 Missing and 2 partials ⚠️
overlord/hookstate/hooks.go	7.69%	12 Missing ⚠️
snap/info.go	75.67%	6 Missing and 3 partials ⚠️
interfaces/connection.go	89.74%	2 Missing and 2 partials ⚠️
cmd/snap-exec/main.go	90.62%	2 Missing and 1 partial ⚠️
interfaces/snap_app_set.go	91.66%	2 Missing and 1 partial ⚠️
overlord/hookstate/hookmgr.go	86.36%	2 Missing and 1 partial ⚠️
... and 3 more

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #13976      +/-   ##
==========================================
- Coverage   78.86%   78.71%   -0.15%     
==========================================
  Files        1043     1050       +7     
  Lines      134595   137490    +2895     
==========================================
+ Hits       106144   108232    +2088     
- Misses      21837    22476     +639     
- Partials     6614     6782     +168

Flag	Coverage Δ
unittests	`78.71% <78.61%> (-0.15%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pedronis

did a first pass, bunch of questions

pedronis · 2024-06-18T16:13:41Z

snap/info_test.go

+
+	// TODO: fix this annoying import cycle issue
+	container := func(p string) (snap.Container, error) {
+		return snapdir.New(p), nil


you can use a hook from snapdir into snap I suppose to fix this?

Addressed in 1e559b9, first timing adding one of those, so might be wrong.

pedronis · 2024-06-18T16:14:47Z

snap/info.go

+
+	revision, err := ParseRevision(rev)
+	if err != nil {
+		return nil, fmt.Errorf("cannot read revision %s: %s", rev, err)


s/read/parse/ ? also this error message doesn't seem to contain a lot of context

Addressed in 4b1b48a.

snap/snaptest/snaptest.go

pedronis · 2024-06-18T16:28:33Z

cmd/snap/cmd_run.go

+	if hook != nil {
+		snapName, _ = snap.SplitSnapComponentInstanceName(snapTarget)
+	} else {
+		snapName, _ = snap.SplitSnapApp(snapTarget)
+	}


I don't understand the if here ? do we need to make the difference if snap.SplitSnapComponentInstanceName works in all cases?

I think this was handled better by the refactor in the following commit.

pedronis · 2024-06-18T16:31:09Z

cmd/snap/cmd_run.go

 	snapConfine, err := snapdHelperPath("snap-confine")
 	if err != nil {
 		return err
 	}
 	if !osutil.FileExists(snapConfine) {
-		if hook != nil {
-			logger.Noticef("WARNING: skipping running hook %q of snap %q: missing snap-confine", hook.Name, info.InstanceName())
+		if runner.Hook() != nil {


have a IsHook predicate as well instead?

Added in bfc4a0d.

pedronis · 2024-06-18T16:34:09Z

cmd/snap-exec/main.go

 	}

-	return execApp(snapApp, revision, opts.Command, extraArgs)
+	return execApp(snapName, revision, opts.Command, extraArgs)


I would have expected snapTarget here?? missing tests or am I confused?

Addressed in 79aad91. I moved the parsing of the target into the execHook and execApp functions themselves, to hopefully make things a bit easier to read.

alfonsosanchezbeato

Couple of comments below

alfonsosanchezbeato · 2024-06-18T16:25:48Z

snap/info.go

@@ -1585,6 +1585,32 @@ func ReadCurrentInfo(snapName string) (*Info, error) {
 	return ReadInfo(snapName, &SideInfo{Revision: revision})
 }

+func ReadCurrentComponentInfo(component string, info *Info, container func(string) (Container, error)) (*ComponentInfo, error) {
+	link := filepath.Join(ComponentsBaseDir(info.InstanceName()), info.Revision.String(), component)


This could use snap.ComponentLinkPath() now that #14060 has been merged.

That function takes a ContainerPlaceInfo, which will be a componentPlaceInfo. That type's constructor expects a revision, which we don't have here.

snap.ComponentLinkPath() could just take a the component's name, which is the only thing that is used here:

func ComponentLinkPath(cpi ContainerPlaceInfo, snapRev Revision) string { instanceName, compName, _ := strings.Cut(cpi.ContainerName(), "+") compBase := ComponentsBaseDir(instanceName) return filepath.Join(compBase, snapRev.String(), compName) }

You fine with me making that change, so that I can use the function here?

Sure, that is fine

alfonsosanchezbeato · 2024-06-18T17:51:01Z

cmd/snap/cmd_run.go

+		// TODO: we need to figure out how to get the component revision to set
+		// the environment variables that we provide to the hook
+		componentRevision := snap.Revision{}
+		_ = componentRevision


Maybe we need a --comprev argument as we are using -r for the snap revision for hooks, and use it in runHookAndWait. Not sure though why we do that instead of reading the snap information as in the non-hook case, probably @pedronis knows.

there was the idea to run hooks for the non-current revisions but I'm not sure it can work in practice. maybe we should revisit this

Sorry, this TODO is old and I forgot to remove it. We are able to get the revision of the component ever since the change that @alfonsosanchezbeato made with the component symlinks. Although yes, I'm not sure what to do with running component hooks for revisions of a component that is not the current one. I'm not sure how that would work? What is the use case for that?

pedronis

thanks for the changes, some questions

pedronis · 2024-06-19T16:36:38Z

cmd/snap/cmd_run.go

+		// TODO: we need to figure out how to get the component revision to set
+		// the environment variables that we provide to the hook
+		componentRevision := snap.Revision{}
+		_ = componentRevision


there was the idea to run hooks for the non-current revisions but I'm not sure it can work in practice. maybe we should revisit this

snap/info.go

cmd/snap-exec/main.go

cmd/snap/cmd_run.go

pedronis

thank you, couple small comments

pedronis · 2024-06-21T14:46:59Z

snap/component.go

-func ComponentLinkPath(cpi ContainerPlaceInfo, snapRev Revision) string {
-	instanceName, compName, _ := strings.Cut(cpi.ContainerName(), "+")
+func ComponentLinkPath(cref naming.ComponentRef, snapRev Revision) string {
+	instanceName, compName, _ := strings.Cut(cref.String(), "+")


do we need the Cut? we could use cref.SnapName and cref.ComponentName, no?

Whoops, that was part of the motivation for this change.

Actually, I'm realizing now that this change might be wrong. I believe naming.ComponentRef shouldn't ever carry a snap's instance name, just the regular name? @alfonsosanchezbeato is that correct?

That's correct, it should carry the snap name, not the instance name in principle.

pedronis · 2024-06-21T14:49:41Z

snap/snapdir/snapdir.go

@@ -211,3 +211,11 @@ func (s *SnapDir) ListDir(path string) ([]string, error) {
 func (s *SnapDir) Unpack(src, dstDir string) error {
 	return fmt.Errorf("unpack is not supported with snaps of type snapdir")
 }
+
+func NewContainerFromDir(path string) snap.Container {


probably want a doc comment on this

alfonsosanchezbeato

LGTM, thanks for the changes

pedronis

thanks, some small comments and a nitpick

pedronis · 2024-06-24T14:26:29Z

snap/info.go

+	// able to set the revision of the component. we create it so that we can
+	// use ComponentLinkPath, which doesn't use the revision.
+	cpi := MinimalComponentContainerPlaceInfo(component, Revision{}, info.InstanceName())
+


nitpick: I would leave this blank line

pedronis · 2024-06-24T14:26:45Z

snap/info.go

-	link := filepath.Join(ComponentsBaseDir(info.InstanceName()), info.Revision.String(), component)
+	// NOTE: creating this here is a bit of a hack, since we aren't actually
+	// able to set the revision of the component. we create it so that we can
+	// use ComponentLinkPath, which doesn't use the revision.


does the ComponentLinkPath doc mention is not consuming the revision?

pedronis · 2024-06-24T14:27:35Z

snap/info.go

@@ -1594,7 +1594,12 @@ var NewContainerFromDir func(snapName string) Container = func(snapName string)
 // ReadCurrentComponentInfo reads the ComponentInfo for the currently linked
 // revision of the given component associated with the given snap.
 func ReadCurrentComponentInfo(component string, info *Info) (*ComponentInfo, error) {
-	link := filepath.Join(ComponentsBaseDir(info.InstanceName()), info.Revision.String(), component)
+	// NOTE: creating this here is a bit of a hack, since we aren't actually


s/NOTE/TODO, when we have a bit more time or other use cases we should reconsider this

…urrent revision of a component for a snap revision

…resent snap hooks, component hooks, and apps This commit doesn't need to be here, and things will work without it. But things were getting a bit complicated in runSnapConfine with arguments that represented different things based on what we were running.

…sion a bit better

…r into snap

…ally

…tallDate param with naming.ComponentRef

This reverts commit 9a56c37.

…onentInstallDate param with naming.ComponentRef" This reverts commit ca39dc1.

This reverts commit a3a9130.

…aints of the ContainerPlaceInfo param

github-actions bot added the Needs Documentation -auto- Label automatically added which indicates the change needs documentation label May 17, 2024

andrewphelpsj requested review from pedronis and alfonsosanchezbeato May 17, 2024 08:50

andrewphelpsj force-pushed the component-snap-run branch 3 times, most recently from b06f281 to 0523065 Compare May 23, 2024 16:24

andrewphelpsj force-pushed the component-snap-run branch 2 times, most recently from 85f919d to 6878d5b Compare June 11, 2024 18:17

github-actions bot removed the Needs Documentation -auto- Label automatically added which indicates the change needs documentation label Jun 11, 2024

andrewphelpsj force-pushed the component-snap-run branch from 6878d5b to 4e1002a Compare June 13, 2024 15:01

andrewphelpsj force-pushed the component-snap-run branch 2 times, most recently from fe158d1 to 02b71cf Compare June 14, 2024 16:44

pedronis reviewed Jun 18, 2024

View reviewed changes

andrewphelpsj force-pushed the component-snap-run branch from 1e559b9 to b099df9 Compare June 18, 2024 17:27

andrewphelpsj requested a review from pedronis June 18, 2024 17:27

alfonsosanchezbeato reviewed Jun 18, 2024

View reviewed changes

pedronis reviewed Jun 19, 2024

View reviewed changes

andrewphelpsj requested review from pedronis and alfonsosanchezbeato June 20, 2024 16:31

github-actions bot added the Needs Documentation -auto- Label automatically added which indicates the change needs documentation label Jun 20, 2024

pedronis reviewed Jun 21, 2024

View reviewed changes

andrewphelpsj requested a review from pedronis June 21, 2024 19:02

github-actions bot removed the Needs Documentation -auto- Label automatically added which indicates the change needs documentation label Jun 21, 2024

alfonsosanchezbeato approved these changes Jun 21, 2024

View reviewed changes

pedronis approved these changes Jun 24, 2024

View reviewed changes

andrewphelpsj added 4 commits June 25, 2024 09:07

snap, s/snaptest: add function for reading the ComponentInfo of the c…

1c1eff0

…urrent revision of a component for a snap revision

s/snapenv, c/snap: add support for component hooks to ExtendEnvForRun

0ec7028

c/snap: update run to be able to run component hooks

f7a47d9

andrewphelpsj added 21 commits June 25, 2024 09:07

c/snap-exec: handle running component hooks in snap-exec

01b9ca4

c/snap-exec: move parsing of snap-exec target into execHook and execApp

89f0c73

snap: make error message when failing to parse current component revi…

d915e47

…sion a bit better

c/snap: add IsHook method to runnable type for easier checking

d9f2e7b

s/snaptest: use os.Symlink rather than atomic variant in test code

29ed42f

snap, s/snapdir, c/snap: fix import cycle issue with hook from snapdi…

82193ed

…r into snap

c/snap, c/snap-exec: docs and panicking default for NewContainerFromDir

61dc73b

c/snap, c/snap-exec: set up hook for snap.NewContainerFromDir

d3b470f

c/snap: remove TODO about getting component revision

699801b

c/snap, c/snap-exec: use _ imports rather than initializing hook manu…

db0e0f9

…ally

s/naming: add ParseComponentRef function

60cbe40

snap, o/s/backend, daemon: replace ComponentLinkPath and ComponentIns…

746b8a9

…tallDate param with naming.ComponentRef

snap: use ComponentLinkPath helper in ComponentLinkPath

8da36fb

s/snapdir: add doc comment for NewContainerForDir

1f1d108

Revert "snap: use ComponentLinkPath helper in ComponentLinkPath"

c467eec

This reverts commit 9a56c37.

Revert "snap, o/s/backend, daemon: replace ComponentLinkPath and Comp…

0a372fd

…onentInstallDate param with naming.ComponentRef" This reverts commit ca39dc1.

Revert "s/naming: add ParseComponentRef function"

a31cd6f

This reverts commit a3a9130.

snap: use ComponentLinkPath helper in ComponentLinkPath

0bfe8d5

snap: remove whitespace

9b74ba1

snap: update doc comment on ComponentLinkPath to mention usage constr…

163cc7c

…aints of the ContainerPlaceInfo param

snap: replace NOTE with TODO

715fed6

andrewphelpsj force-pushed the component-snap-run branch from 352811e to 715fed6 Compare June 25, 2024 13:09

alfonsosanchezbeato merged commit 90c2e4c into canonical:master Jun 26, 2024
44 of 51 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

many: modify snap run to understand component hooks #13976

many: modify snap run to understand component hooks #13976

andrewphelpsj commented May 17, 2024 •

edited

Loading

codecov-commenter commented Jun 13, 2024

pedronis left a comment

pedronis Jun 18, 2024

andrewphelpsj Jun 18, 2024

pedronis Jun 18, 2024

andrewphelpsj Jun 18, 2024

pedronis Jun 18, 2024

andrewphelpsj Jun 18, 2024

pedronis Jun 18, 2024

andrewphelpsj Jun 18, 2024

pedronis Jun 18, 2024

andrewphelpsj Jun 18, 2024

alfonsosanchezbeato left a comment

alfonsosanchezbeato Jun 18, 2024

andrewphelpsj Jun 20, 2024

alfonsosanchezbeato Jun 20, 2024

alfonsosanchezbeato Jun 18, 2024

pedronis Jun 19, 2024

andrewphelpsj Jun 20, 2024

pedronis left a comment

pedronis Jun 19, 2024

pedronis left a comment

pedronis Jun 21, 2024

andrewphelpsj Jun 21, 2024

andrewphelpsj Jun 21, 2024

alfonsosanchezbeato Jun 21, 2024

pedronis Jun 21, 2024

alfonsosanchezbeato left a comment

pedronis left a comment

pedronis Jun 24, 2024

pedronis Jun 24, 2024

pedronis Jun 24, 2024

many: modify snap run to understand component hooks #13976

many: modify snap run to understand component hooks #13976

Conversation

andrewphelpsj commented May 17, 2024 • edited Loading

codecov-commenter commented Jun 13, 2024

Codecov Report

pedronis left a 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

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

alfonsosanchezbeato left a 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

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis left a 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

alfonsosanchezbeato left a comment

Choose a reason for hiding this comment

pedronis left a 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

andrewphelpsj commented May 17, 2024 •

edited

Loading