cmd/snap: record snap-run-inhibit notice #13770

ZeyadYasser · 2024-03-28T18:32:37Z

This PR builds on #13791 (~~and should be rebased after #13791 gets merged~~ already rebased) to enable the "snap-run during refresh UX" flow described in SD167.

This simply allows snap run to issue (per-user) snap-run-inhibit notices when it is inhibited for refresh. This notice can later be consumed by something like snapd-desktop-integration snap replacing the old flow of directly calling snapd-desktop-integration on dbus.

As fallback if no snap exists with the marker snap-refresh-observe interface #13619, a normal notification is sent using the user agent.

NOTE: The graphical flow (using notices or the fallback) is hidden behind the refresh-app-awareness-ux experimental flag #13479.
NOTE: depending if #13747 lands first or this PR, the tests structure will need to be updated because they touch similar parts of the tests.

codecov-commenter · 2024-03-29T12:13:28Z

Codecov Report

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

Project coverage is 78.91%. Comparing base (7b30c56) to head (0b632dc).
Report is 7 commits behind head on master.

Files	Patch %	Lines
cmd/snap/inhibit.go	72.09%	11 Missing and 1 partial ⚠️
daemon/api_notices.go	77.50%	6 Missing and 3 partials ⚠️
client/notices.go	72.72%	4 Missing and 2 partials ⚠️
daemon/snap.go	70.00%	2 Missing and 1 partial ⚠️
cmd/snap/cmd_run.go	66.66%	0 Missing and 1 partial ⚠️

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

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #13770      +/-   ##
==========================================
- Coverage   78.92%   78.91%   -0.01%     
==========================================
  Files        1043     1044       +1     
  Lines      134425   134510      +85     
==========================================
+ Hits       106089   106154      +65     
- Misses      21722    21732      +10     
- Partials     6614     6624      +10

Flag	Coverage Δ
unittests	`78.91% <73.94%> (-0.01%)`	⬇️

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.

zyga · 2024-04-05T08:26:27Z

client/notices.go

@@ -0,0 +1,64 @@
+// Copyright (c) 2024 Canonical Ltd


I'm commenting on the first line that this patch should be split.

The changes to client are stand-alone and can be proposed straight way.

The changes to cmd/snap should be split out and can depend on the larger patch-set.

I created #13791 for the client changes. This PR needs both #13791 and #13769 to be functional.

I will rebase current PR on top of the client PR #13791, and mark it as draft until snap-run-inhibit notice PR #13769 is merged.

Updated, Thank you!

pedronis

general question about the old code

pedronis · 2024-04-10T12:35:48Z

cmd/snap/inhibit.go

+		// no snap (i.e. snapd-desktop-integration) is listening, let's fallback
+		// to normal graphical flow.
+		return false, nil
+	}


the original graphical flow never really worked before, so not sure we should keep it?

Great point, better to remove it. Thanks!

Removed, thank you!

bboozzoo

LGTM

zyga · 2024-04-11T08:33:04Z

As discussed yesterday, my only concern is about compatibility for all the. Releases of Ubuntu who are upgrading to the snapd? You may not have the special desktop snap, which enables the notices to be displayed.

Perhaps we can consider the case of a display enabled user without the special interface connected, using a tty, to display message linking to something on the forum.

cmd/snap/inhibit.go

pedronis · 2024-04-11T09:37:25Z

cmd/snap/inhibit.go

-		return &graphicalFlow{instanceName: instanceName}
+var newInhibitionFlow = func(cli *client.Client, instanceName string) inhibitionFlow {
+	if isGraphicalFlowSupported(cli) {
+		return &graphicalFlow{instanceName: instanceName, cli: cli}


shouldn't we simply always emit the notice but do the fallback if there is no connection etc?

Yes, good point

pedronis

question

oops

ZeyadYasser · 2024-04-11T12:09:31Z

As discussed yesterday, my only concern is about compatibility for all the. Releases of Ubuntu who are upgrading to the snapd? You may not have the special desktop snap, which enables the notices to be displayed.

Perhaps we can consider the case of a display enabled user without the special interface connected, using a tty, to display message linking to something on the forum.

@zyga according to https://ubuntu-archive-team.ubuntu.com/seeds/ubuntu.jammy/desktop-minimal

Desktop snaps: these also exist in ubuntu-release-upgrader's DistUpgradeQuirks.py for users who upgrade.

It seems that installing snapd-desktop-integration is a part of the upgrade process.

ZeyadYasser · 2024-04-11T12:25:34Z

@zyga @pedronis @bboozzoo
The flow is updated to always emit a notice, and fallback to showing a text message if no marker interface is connected or if no terminal is detected.

I will updated the spread test shortly.

pedronis

thanks

this has changed again a bit

bboozzoo · 2024-04-11T12:41:53Z

cmd/snap/inhibit.go

+	// Fallback to text notification if marker "snap-refresh-observe"
+	// interface is not connected.
+	if isStdoutTTY && !markerInterfaceConnected(gf.cli) {
+		fmt.Fprintf(Stdout, i18n.G("snap package %q is being refreshed, please wait\n"), gf.instanceName)


are there cases where regardless of sitting on a tty we'd like to produce the message to stderr? TBH even with the current check's I'd still write this to stderr

Good point, I will update it.

updated, thank you!

ZeyadYasser · 2024-04-11T16:56:23Z

I removed the text fallback spread test variant to avoid blocking this PR further because it was inconsistent across systems due to the terminal checks in snapd. It is hard to mock a real terminal in all systems while redirecting output to a file for testing.

The extra variant can be added in a follow up PR when I come up with a better way to trick snap command it is running in a terminal while redirecting output for parsing.

@pedronis

Record a snap-run-inhibit notice when snap run is inhibited due refresh. * cmd/snap: remove old desktop notifications (thanks @pedronis @zyga) * cmd/snap: always send notices when snap run is inhibited + fallback to text if no snap has the marker snap-refresh-observe interface connected and a terminal is detected. * cmd/snap: send text fallback notification to stderr (thanks @bboozzoo) * cmd/snap: initialize inhibition flow only when it is needed Signed-off-by: Zeyad Gouda <[email protected]>

* test/main/snap-run-inhibition-flow: remove text fallback check Text fallback is inconsistent across systems due to the terminal checks in snapd. It is hard to mock a real terminal in all systems while redirecting output to a file for testing. Signed-off-by: Zeyad Gouda <[email protected]>

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

ZeyadYasser mentioned this pull request Mar 28, 2024

o/state,daemon: add snap-run-inhibit notice #13769

Merged

ZeyadYasser requested review from bboozzoo, zyga and pedronis March 28, 2024 18:50

ZeyadYasser added the Needs Samuele review Needs a review from Samuele before it can land label Mar 28, 2024

ZeyadYasser marked this pull request as ready for review March 28, 2024 18:54

ZeyadYasser added this to the 2.63 milestone Mar 29, 2024

ZeyadYasser mentioned this pull request Apr 1, 2024

many: refresh-app-awareness UX improvement/enablement [WIP] #13066

Closed

30 tasks

ZeyadYasser force-pushed the record-snap-run-inhibit-notice branch from 0b632dc to f93ea05 Compare April 3, 2024 20:43

zyga reviewed Apr 5, 2024

View reviewed changes

ZeyadYasser mentioned this pull request Apr 5, 2024

client: add support to send notices #13791

Merged

ZeyadYasser force-pushed the record-snap-run-inhibit-notice branch from f93ea05 to 74ed28c Compare April 5, 2024 11:48

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

ZeyadYasser marked this pull request as draft April 5, 2024 11:48

ZeyadYasser requested a review from zyga April 5, 2024 11:53

ZeyadYasser changed the title ~~cmd/snap,client: record snap-run-inhibit notice~~ cmd/snap: record snap-run-inhibit notice Apr 5, 2024

ZeyadYasser added the ⛔ Blocked label Apr 8, 2024

ZeyadYasser marked this pull request as ready for review April 8, 2024 17:00

ZeyadYasser force-pushed the record-snap-run-inhibit-notice branch from 74ed28c to bbc4050 Compare April 10, 2024 10:16

ZeyadYasser removed the ⛔ Blocked label Apr 10, 2024

pedronis reviewed Apr 10, 2024

View reviewed changes

ZeyadYasser requested a review from pedronis April 10, 2024 17:06

bboozzoo previously approved these changes Apr 11, 2024

View reviewed changes

pedronis reviewed Apr 11, 2024

View reviewed changes

cmd/snap/inhibit.go Outdated Show resolved Hide resolved

pedronis reviewed Apr 11, 2024

View reviewed changes

pedronis previously approved these changes Apr 11, 2024

View reviewed changes

pedronis self-requested a review April 11, 2024 11:35

pedronis approved these changes Apr 11, 2024

View reviewed changes

pedronis requested a review from bboozzoo April 11, 2024 12:27

bboozzoo approved these changes Apr 11, 2024

View reviewed changes

ZeyadYasser force-pushed the record-snap-run-inhibit-notice branch from a0e7915 to a6dcbd9 Compare April 11, 2024 19:33

ZeyadYasser added 2 commits April 11, 2024 21:33

ZeyadYasser force-pushed the record-snap-run-inhibit-notice branch from a6dcbd9 to 9f15777 Compare April 11, 2024 19:34

ernestl merged commit b3c05c9 into canonical:master Apr 12, 2024
41 of 43 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/snap: record snap-run-inhibit notice #13770

cmd/snap: record snap-run-inhibit notice #13770

ZeyadYasser commented Mar 28, 2024 •

edited

Loading

codecov-commenter commented Mar 29, 2024

zyga Apr 5, 2024

ZeyadYasser Apr 5, 2024

ZeyadYasser Apr 5, 2024

pedronis left a comment

pedronis Apr 10, 2024

ZeyadYasser Apr 10, 2024

ZeyadYasser Apr 10, 2024

bboozzoo left a comment

zyga commented Apr 11, 2024

pedronis Apr 11, 2024

ZeyadYasser Apr 11, 2024

pedronis left a comment

ZeyadYasser commented Apr 11, 2024

ZeyadYasser commented Apr 11, 2024

pedronis left a comment

bboozzoo Apr 11, 2024

ZeyadYasser Apr 11, 2024

ZeyadYasser Apr 11, 2024

ZeyadYasser commented Apr 11, 2024

cmd/snap: record snap-run-inhibit notice #13770

cmd/snap: record snap-run-inhibit notice #13770

Conversation

ZeyadYasser commented Mar 28, 2024 • edited Loading

codecov-commenter commented Mar 29, 2024

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bboozzoo left a comment

Choose a reason for hiding this comment

zyga commented Apr 11, 2024

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

ZeyadYasser commented Apr 11, 2024

ZeyadYasser commented Apr 11, 2024

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

ZeyadYasser commented Apr 11, 2024

ZeyadYasser commented Mar 28, 2024 •

edited

Loading