many: add a *SnapAppSet to ConnectedPlug/Slot types and use it to build label expressions in interfaces #13773

andrewphelpsj · 2024-04-01T18:28:50Z

Now that we have app sets in the interfaces repo, keep a pointer to them in ConnectedPlug/Slot types. Use this to build label expressions in the interfaces. Based on #13772, first relevant commit is 4985f65.

codecov-commenter · 2024-04-04T17:20:41Z

Codecov Report

Attention: Patch coverage is 58.60735% with 214 lines in your changes are missing coverage. Please review.

Project coverage is 78.74%. Comparing base (1bad859) to head (db5f741).
Report is 15 commits behind head on master.

Files	Patch %	Lines
interfaces/ifacetest/backendtest.go	0.00%	64 Missing ⚠️
overlord/ifacestate/handlers.go	48.30%	42 Missing and 19 partials ⚠️
interfaces/ifacetest/app_set.go	38.46%	30 Missing and 2 partials ⚠️
overlord/ifacestate/helpers.go	45.71%	12 Missing and 7 partials ⚠️
interfaces/repo.go	61.70%	12 Missing and 6 partials ⚠️
interfaces/connection.go	33.33%	8 Missing ⚠️
interfaces/snap_app_set.go	93.24%	4 Missing and 1 partial ⚠️
interfaces/kmod/backend.go	71.42%	0 Missing and 2 partials ⚠️
daemon/api_sideload_n_try.go	0.00%	1 Missing ⚠️
interfaces/builtin/modem_manager.go	50.00%	1 Missing ⚠️
... and 3 more

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

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #13773      +/-   ##
==========================================
- Coverage   78.83%   78.74%   -0.09%     
==========================================
  Files        1043     1044       +1     
  Lines      134470   135006     +536     
==========================================
+ Hits       106012   106314     +302     
- Misses      21847    22039     +192     
- Partials     6611     6653      +42

Flag	Coverage Δ
unittests	`78.74% <58.60%> (-0.09%)`	⬇️

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.

…nd snapd snaps

… interfaces can build a complete label expression, including component hooks

alfonsosanchezbeato

LGTM, thanks (I looked at 0356d2d)

pedronis

did a pass on the last commit

pedronis · 2024-06-07T15:46:24Z

interfaces/connection.go

 	staticAttrs  map[string]interface{}
 	dynamicAttrs map[string]interface{}
 }

+// Apps returns all the apps associated with this plug.


this doc comment doesn't quite match the name and purpose of the method

pedronis · 2024-06-07T15:46:40Z

interfaces/connection.go

@@ -139,8 +156,8 @@ func (plug *ConnectedPlug) Snap() *snap.Info {
 }

 // Apps returns all the apps associated with this plug.
-func (plug *ConnectedPlug) Apps() map[string]*snap.AppInfo {
-	return plug.plugInfo.Apps
+func (plug *ConnectedPlug) AppSet() *SnapAppSet {


pedronis · 2024-06-07T15:47:46Z

interfaces/connection.go

I just noticed that this file doesn't respect go conventions, it mixes the definition of the ConnectedPlug and ConnectedSlot methods, instead of having those follow their struct introduction. Maybe we should change this in an immediate follow up

pedronis · 2024-06-07T15:54:35Z

interfaces/snap_app_set.go

 // constructed from the apps and hooks that are associated with the plug.
-func (a *SnapAppSet) PlugLabelExpression(plug *ConnectedPlug) string {
+func (a *SnapAppSet) plugLabelExpression(plug *ConnectedPlug) string {
 	// TODO: this is a hack that will not continue to work once component hooks
 	// are introduced. the methods on SnapAppSet should only be called on
 	// slots/hooks that originated from the snap that the SnapAppSet was derived
 	// from.


is the TODO still relevant?

pedronis · 2024-06-07T15:55:18Z

interfaces/snap_app_set.go

 	// TODO: this is a hack that will not continue to work once component hooks
 	// are introduced. the methods on SnapAppSet should only be called on
 	// slots/hooks that originated from the snap that the SnapAppSet was derived
 	// from.
-	info := a.info
 	if a.info.InstanceName() != plug.plugInfo.Snap.InstanceName() {


shouldn't this use plug.appSet now? and the other check go to the constructors of ConnectedPlug?

pedronis · 2024-06-07T15:55:35Z

interfaces/snap_app_set.go

 	if a.info.InstanceName() != slot.slotInfo.Snap.InstanceName() {
-		info = slot.slotInfo.Snap


same comments

pedronis · 2024-06-07T15:56:22Z

interfaces/snap_app_set.go

+func appRunnable(app *snap.AppInfo) Runnable {
+	return Runnable{
+		CommandName: app.Name,
+		SecurityTag: app.SecurityTag(),
+	}
+}
+
+func hookRunnable(hook *snap.HookInfo) Runnable {


maybe Runnable and this belong in the snap package?

pedronis · 2024-06-07T15:59:36Z

interfaces/snap_app_set.go

+			names = append(names, fmt.Sprintf("+%s.hook.%s", hook.Component.Name, hook.Name))
+		} else {
+			names = append(names, fmt.Sprintf(".hook.%s", hook.Name))


thought: I wonder if there's a way to rewrite this using Runnables() and some more methods on that?

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

andrewphelpsj mentioned this pull request Apr 1, 2024

interfaces/builtin, snap: remove sanitized plugs from component hook plugs #13774

Merged

andrewphelpsj force-pushed the app-set-in-connected branch 2 times, most recently from f9e50ab to 3c1cc60 Compare April 2, 2024 14:51

andrewphelpsj requested review from alfonsosanchezbeato and pedronis April 4, 2024 15:23

andrewphelpsj force-pushed the app-set-in-connected branch 2 times, most recently from 92d8bc4 to db5f741 Compare April 4, 2024 16:48

andrewphelpsj force-pushed the app-set-in-connected branch 2 times, most recently from 19d258f to 8915966 Compare April 8, 2024 17:41

andrewphelpsj force-pushed the app-set-in-connected branch 2 times, most recently from fdf150a to 6e0146e Compare April 16, 2024 19:18

andrewphelpsj mentioned this pull request Apr 17, 2024

many: run component install hooks #13775

Open

andrewphelpsj force-pushed the app-set-in-connected branch 2 times, most recently from 4985f65 to ca4f30f Compare April 17, 2024 15:22

andrewphelpsj force-pushed the app-set-in-connected branch 3 times, most recently from a31505b to 4a60a79 Compare May 7, 2024 14:41

andrewphelpsj force-pushed the app-set-in-connected branch 2 times, most recently from c2179fb to 22cf201 Compare May 15, 2024 07:43

andrewphelpsj force-pushed the app-set-in-connected branch 3 times, most recently from c94234c to 05db36c Compare May 23, 2024 16:25

andrewphelpsj added 6 commits June 4, 2024 17:30

many: replace interfaces.Repo.AddSnap with AddAppSet

b0dc4d7

interfaces: remove repo.RemovePlug since it is unused

9e6c0bd

interfaces: simplify check for a snap's presence in interfaces repo

afbb084

fixup! many: replace interfaces.Repo.AddSnap with AddAppSet

2eb2915

interfaces: update doc comment on Repository.AddAppSet

8a1f16e

o/ifacestate: fix duplicate init of app set following refactor in master

31d111d

andrewphelpsj added 2 commits June 5, 2024 13:54

o/snapstate: fix bug that caused implicit slots to be added to core a…

a37f618

…nd snapd snaps

many: add a pointer to a SnapAppSet into Connected(Plug|Slot) to that…

0356d2d

… interfaces can build a complete label expression, including component hooks

andrewphelpsj force-pushed the app-set-in-connected branch from 05db36c to 0356d2d Compare June 5, 2024 19:00

alfonsosanchezbeato approved these changes Jun 6, 2024

View reviewed changes

pedronis reviewed Jun 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

many: add a *SnapAppSet to ConnectedPlug/Slot types and use it to build label expressions in interfaces #13773

many: add a *SnapAppSet to ConnectedPlug/Slot types and use it to build label expressions in interfaces #13773

andrewphelpsj commented Apr 1, 2024 •

edited

codecov-commenter commented Apr 4, 2024

alfonsosanchezbeato left a comment

pedronis left a comment

pedronis Jun 7, 2024

pedronis Jun 7, 2024

pedronis Jun 7, 2024

pedronis Jun 7, 2024

pedronis Jun 7, 2024

pedronis Jun 7, 2024

pedronis Jun 7, 2024

pedronis Jun 7, 2024

		if a.info.InstanceName() != slot.slotInfo.Snap.InstanceName() {
		info = slot.slotInfo.Snap

many: add a *SnapAppSet to ConnectedPlug/Slot types and use it to build label expressions in interfaces #13773

Are you sure you want to change the base?

many: add a *SnapAppSet to ConnectedPlug/Slot types and use it to build label expressions in interfaces #13773

Conversation

andrewphelpsj commented Apr 1, 2024 • edited

codecov-commenter commented Apr 4, 2024

Codecov Report

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

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

andrewphelpsj commented Apr 1, 2024 •

edited