* usersession: Use the app name from .desktop file in notifications
Currently, when a notification is shown to the user to inform
they that there is an update for an open snap, the snap name is
used, which can be misleading to the user.
This PR uses the application name from the .desktop file, taking
into account the current locale of the user.
* Add tests for localized name getter
* Take into account the instance key
* Change variable name
* Code simplification
* Try to pass the static analyzer
* Revert "Try to pass the static analyzer"
This reverts commit 3c031bef098d1a1fab6afad02aa456a80fb84154.
* Another try to pass the static analyzer
* Revert "Another try to pass the static analyzer"
This reverts commit 39a37d9e129d1220566e7da9e4be4dd2d3f7324f.
* Added extra tests to check notification text
These four tests ensure that the notification text is correct
when there is an instance key and/or there is a .desktop file
from where a translatable name can be obtained.
* Renamed TestGuessAppIcon... to TestGuessAppData...
* agentnotify: show the snap icon when autorefresh is done
When an application has to be closed by the user to allow to do
and autorefresh, the notification has the application icon. But
when the autorefresh has been completed, the notification has
the snapcraft icon instead.
This MR fixes this. With it, the notification specifying that
the snap has been successfully refreshed and that it can be
relaunched shows the application's icon.
Fix https://warthogs.atlassian.net/browse/UDENG-2030
* Fix .desktop file name generation
* Use an heuristic to determine the desktop file
* Move all the logic to the user-space daemon
* Don't fail if ReadCurrentInfo fails
Also, really take the first icon found, and not the last one.
* Moved code into function and added tests
* Update usersession/agent/rest_api.go
Co-authored-by: Maciej Borzecki <maciek.borzecki@gmail.com>
* Update usersession/agent/rest_api.go
Co-authored-by: Maciej Borzecki <maciek.borzecki@gmail.com>
* Extra changes requested by reviewers
* Add extra test
* Extra changes requested
* Update usersession/agent/rest_api_test.go
Co-authored-by: Maciej Borzecki <maciek.borzecki@gmail.com>
* Some style patches
---------
Co-authored-by: Maciej Borzecki <maciek.borzecki@gmail.com>
Some invocations to external programs used exec.CombinedOutput, that
combines stdout and strerr into a single byte array. This can be an
issue if this output is parsed, as many programs print debug output or
warnings to stderr and that data is unexpected by the parsers. This
patch changes to using osutil.RunSplitOutput or osutil.RunCmd (that
return separately stdout and stderr) when we need to parse stdout, and
also in some other cases when printing separately both streams could
be helpful. Fixes LP #1885597.
Replace ioutil.WriteFile with os.WriteFile since the former has been
deprecated since go1.16 and simply calls the latter.
Signed-off-by: Miguel Pires <miguel.pires@canonical.com>
On Ubuntu 16.04, ssh sessions may not have a D-Bus session bus,
resulting in the `bus` field of the session agent being nil. If this is
the case, calling `s.bus.Close()` results in a nil pointer exception.
This commit adds a check of `s.bus` (like those found elsewhere in
session_agent.go) before calling `s.bus.Close()` during a session
shutdown.
Discovered-by: Andrew Phelps <andrew.phelps@canonical.com>
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
* snapstate: notify user when refresh-app-awareness snap refreshed
This is a minimal commit to show a notification when a refresh that
was triggered by the user closing an application is now ready
to use.
* {iface,snap}state: make LinkParticipant pass a *SnapSetup
* snapstate: use notifyLinkParticipant mechanism to trigger finish notification
* agentnotify: move agent notification into their own package
* snapstate: make AutoRefresh() take *AutoRefreshOptions
* agentnotify: import from overlord to ensure linkParticipant handler gets registered
* tests: add new refresh-app-awareness-notify spread test
* snapstate: fix unreachable code
* tests: move from wormhole to test-snapd-sh in refresh-app-awareness-notify
* agentnotify: send notification when snap starts to refresh
* usersession: tweak wording when refresh is ready (thanks to Oliver)
* usersession: actually add the missing finishRefreshNotificationCmd endpoint
* snap: disable graphicalSessionFlow() to avoid confusion
It's unclear if this flow ever worked and the finish-refresh
part for sure never worked because in the userd the API for
finish-notification was not wired in.
This needs to be re-examined when we have a proper UX design.
* usersession: show notification when update is finished (thanks to Oliver)
* agentnotify: remove uneeded notifyUnlinkSnap()
After a quick sync with Oliver the decision was taken to have
just a notification message when the snap refresh is finished
and not when it starts to avoid too many user distractions.
* usersession: tweak wording when refresh is ready (thanks to Oliver)
* agentnotify: remove unneeded comment
* usersession: one more wording tweak (thanks to Oliver)
* agentnotify: add missing unit test
* snapstate: add test that ensures the snapsup.Flags.IsContinuedAutoRefresh is set
* snapstate: add TODO
* many: tweak wordings/comment/variable names (thanks to Miguel)
* tests: simplify refresh-app-awareness-notify test (thanks to Miguel)
* tests: fix quoting in test
* snap: simplify how inhibit worflow inhibited
* tests: tweak tests and ensure "jq" is installed
* agent: fix unit test error
Go 1.19 includes some changes to gofmt which intend to make lists and
heading clearer when rendered (https://go.dev/doc/go1.19). This commit
is the result of running the new gofmt and manually fixing some of it.
This was necessary because the new gofmt assumed lines beginning w/ tabs
to start lists or examples. While this is often true in our codebase,
we occasionally also use tabs to indent the lines after a TODO or FIXME
prefix or in yaml (e.g., excerpts of a snap.yaml). This meant that a lot of the
reformatted comments were broken and had to be fixed manually.
Signed-off-by: Miguel Pires <miguel.pires@canonical.com>
The `rec.HeaderMap` is deprecated since go1.11 and the
`rec.Header()` method should be used instead. This commit
does this switch.
Found by https://staticcheck.io/ which was suggested by Fred,
thanks for this.
This PR is somewhat related to:
https://github.com/snapcore/snapd/pull/11340
In the process of improving the systemd stop code to better track
the progress of stopping units, we also removed the snapd concept
of a timeout period in order to exclusively rely on
'systemctl stop' to return an error after its internal stop
escalation process failed.
We therefore no longer can produce a 'Timeout' error.
This patch removes code in the code base that used to deal
with this 'Timeout' condition.
The idea behind this patch is that we trust 'systemctl stop'
to tell us if it failed to stop a unit within its configured
timeout, and if it fails, there is nothing more we can do but
to return an error.
Signed-off-by: Fred Lotter <fred.lotter@canonical.com>
* systemd: fix snapd systemd-unit stop progress notifications
The current behaviour of the systemd stop progress code is to print
the list of systemd units not stopped after 250ms, and then after
every 20 seconds (notice ticker), erroneously print out the same list
even though some of the units may have stopped in the meantime.
However, the bigger issue is that the non-mocked (real) code starts the
stop sequence by making a blocking systemctl stop request and then
afterwords (once all units are stopped) proceed to the progress code,
which has no effect and exits immediately.
The unit tests does not pick this up because the mock systemctl does
not no anything, and therefore the simulation of progress works as
expected because the output of the 'systemctl show' progress calls are
mocked to simulate certain outcomes.
Finally, the 'Stop()' function provided a 'timeout' argument which
suggested the user could control when a stop attempt should timeout
and fail. However, this feature did not work as the original call was
blocking. Furthermore, this features does not make sense because it
creates a second place where timeouts are introduced. The first place
is in the systemd back-end where 'TimeoutStopSec' is optionally
configured by snapd. In contrast with systemd's actual timeout
mechanism described, the 'Stop()' call offered another timeout, but
this mechanism is inconsistently used all over the code-base with
different hard-coded timeout values, not matching the underlying
service defaults to configured values. In the worse case scenario
the snapd 'Stop()' would return an error due to timeout while systemd
is still busy stopping a unit, because it was a longer unit timeout
configure. The timeout value offered by snapd has no real purpose if
carefully analysed.
The following changes were made:
1. Remove the systemd Stop() timeout argument. The timeout is now
automatically provided by systemdctl when it returns (blocking).
This allows the timeout to always be aligned with the actual
timeout 'TimeoutStopSec' provided by the unit, and takes into
account systemd timeout behaviour related to 'ExecStop' commands.
2. Change the blocking systemctl stop command to async run in a go
routine allowing progress tracking to continue in parallel. Note
that the call to Stop() is still blocking because the progress
code waits until either units are all stopped or the stop command
returns.
3. Change the notice ticker to rather implement an initial notice
silence period (1s). After this expires, the first notification
is generated with the list of units still not stopped. This
delay was chosen based on empirical tests performed on a
Raspberry Pi 3 (representing slower targets) under heavy IO and
CPU load (where it took up to 750ms for 4 services to stop).
4. Change the progress logic to always send a notification after
the notice silence period on any change to the list of unit
we are still waiting for, and always only poll the state of the
remaining units.
5. Add additional unit tests to better cover the new progress and
notification logic, and make existing tests more thorough.
6. Add critical sections inside the mocked systemctl functions to
prevent corruption during concurrent modifications to some of
the test suite logs and counters. This was needed because the
Stop() is now issued from a go routine.
The following example shows the expected output for 6 systemd units
terminated with the 'Stop' function. The last four (test3 - test6)
has a random post SIGTERM delay before the process exits. The following
notifications are generated for this example after the 1s notification
silence period expires:
Waiting for "test3", "test4", "test5", "test6" to stop.
Waiting for "test3", "test4", "test5" to stop.
Waiting for "test3", "test5" to stop.
Waiting for "test3" to stop.
This patch is required by https://github.com/snapcore/snapd/pull/11113
Signed-off-by: Fred Lotter <fred.lotter@canonical.com>
* typos
* Improved the system stop progress logic.
* Number of small review improvements
* Remove redundant channel write
* reviews: add thread safety into MockSystemctl
* many: have a separate systemd.MockSystemctlWithDelay
keep MockSystemctl signature as before
as we need to introduce controlled delays only in the systemd package
own tests so far
Co-authored-by: Samuele Pedroni <pedronis@lucediurna.net>
