snapd icon indicating copy to clipboard operation
snapd copied to clipboard

Published 20 hours ago • verified publisher icon snapcore

Pass error messages to the client on global refresh (bug 1928567)

Open iomezk opened this issue 10 months ago • 10 comments

https://bugs.launchpad.net/snapd/+bug/1928567 https://forum.snapcraft.io/t/refresh-awareness-kills-security/36818

The issue could be partially solved with a minor client-side modification, to behave like snap refresh; snap refresh --list. But it would lack the list of running processes and atomicity.

~~With this commit, snap refresh output structure becomes:~~

[List of updated snaps]
[Error messages]
["All snaps up to date."]

~~Another change is that "All snaps up to date" message will be shown even if all snaps has been successfully refreshed.~~


Build and ./run-checks passed.

iomezk avatar Sep 21 '23 07:09 iomezk

we were thinking of solving the bug using the warning mechanism instead

pedronis avatar Sep 21 '23 08:09 pedronis

It was easy.

But cmd/snap/cmd_warnings.go seems buggy. At least, it outputs wrong number of new warnings.

iomezk avatar Sep 22 '23 12:09 iomezk

@iomezk Thank you for such great work! just a small side note, we usually try to avoid force push after a PR has received reviews to make it easier to incrementally review the changes, for reference please check CONTRIBUTING.md.

ZeyadYasser avatar Sep 26 '23 10:09 ZeyadYasser

@iomezk Thank you for your effort, This looks really good.

We are just missing some unit tests. If you don't have the capacity to write them, I can help. It would be great if we get the PR moving forward.

ZeyadYasser avatar Dec 15 '23 10:12 ZeyadYasser

I can pick this up and add the missing tests.

zyga avatar Mar 06 '24 13:03 zyga

I will rebase this on master to see how it behaves with tests from this year.

zyga avatar Apr 03 '24 10:04 zyga

@zyga I think https://github.com/snapcore/snapd/pull/13780 would solve this issue from another angle (only blocker is that is hidden behind the refresh-app-awareness-ux flag)

ZeyadYasser avatar Apr 03 '24 10:04 ZeyadYasser

Hey! iomezk has not signed the Canonical CLA which is required to get this contribution merged on this project.

Please head over to https://ubuntu.com/legal/contributors to read more about it.

github-actions[bot] avatar Apr 03 '24 11:04 github-actions[bot]

We need to unblock the CLA and we need to discuss how this works with experimental features that are in development. In either case this won't make it for 2.63

zyga avatar Apr 04 '24 13:04 zyga