[05:06] morning [05:55] mborzecki: hi! [05:55] mardy: heya [07:07] morning [07:14] I'm trying to implement Samuele's suggestion from https://github.com/snapcore/snapd/pull/10375#discussion_r656277940, but I'm now getting an error about a cyclic dependency: [07:15] imports github.com/snapcore/snapd/overlord/snapstate [07:15] imports github.com/snapcore/snapd/overlord/snapstate/policy [07:15] imports github.com/snapcore/snapd/overlord/snapstate [07:17] in general, in golang, in which directions should dependency go? from the packages residing in the deeper directory to the more top-level packages? Or is there another logic altogether? [07:20] good morning [07:20] new sprint starts for me today, I will have time to finish spread shellcheck converter (and the lava converter) [07:20] hey pedronis :) [07:20] how are you doing? [07:27] mardy: hey, can you take a look at https://github.com/snapcore/snapd/pull/10444 ? [07:44] mborzecki: +1! [07:44] thx [08:11] what's up with mup? [08:11] no more notices on irc/ [08:11] did mup survive freenode apocalypse? [08:11] oh, mup is gone [08:11] I guess mup is the last user on freenode now :) [08:23] pstolowski: I reviewed https://github.com/snapcore/snapd/pull/10408 , thanks for the changes [08:25] pedronis: thanks! [08:33] pedronis: hi! I don't know if you can see my question above, but when moving the system users to the policy package, I'm getting into a cyclic dependency because the policy package also depends on the snapstate package [08:34] :/ [08:34] pedronis: I've yet to investigate this deeply, but my first impression is that the policy package should not depend on the snapstate package [08:34] it's messy, I forgot [08:35] I'm now going to check what are the things that policy requires from snapstate, and then we can decide how to proceed [08:39] mardy: sorry, let me think a second, there might something easier that can we do [08:40] https://github.com/snapcore/snapd/pull/10442 needs reviews, it's a bit convoluted but it's a refactoring essentially, the real change will come later [08:45] pedronis: so, the policy package uses SnapState, Policy, PolicyFor, NumSnaps, All, Flags, Set from the snapstate package. That's quite a lot of stuff. [08:52] mardy: just make a snap/system_usernames.go instead === pedronis_ is now known as pedronis [09:07] pedronis: thanks :-) [10:01] anyone remembers where was the spread binary that supported `-workers ` ? [10:14] store seems to be a bit unhappy today [10:15] seeing `error: cannot refresh "lxd": unexpectedly empty response from the server (try again later)` [10:38] ehh a bit of a bummer with current tags [10:39] haha [10:39] it was the one from cachio [10:40] custom build with more patches for usability [10:40] mborzecki: can you take a look at this PR from cachio? needs 2nd review: https://github.com/snapcore/snapd/pull/10424 [10:44] pstolowski: I put now https://github.com/snapcore/snapd/pull/7700 into my review queue [10:45] ty [10:48] meh, so it's possible that TAGS is non null, while CURRENT_TAGS is null, eg. when the current set of tags does not match the device anymore, in which case it's impossible to distinguish a case where we're dealing with newer systemd/udev and there are no tags, or the old one where the CURRENT_TAGS did not exist at all [10:50] there's a call in libsystemd device_database_supports_current_tags but it isn't exported, and it's only part of the newer libsystemd, while our builds of snapd snap are done on xenial [10:50] am I right in assuming that the spread failures in https://github.com/snapcore/snapd/pull/10438/checks?check_run_id=2891955009 are not due to my changes? [11:04] mardy looking [11:06] looks like store one one failure [11:06] and I think the apt hooks are problematic, I recall reading about that recently [11:06] I did not read the diff but I think it is unlikely master passes [11:18] zyga-mbp: thanks, I'll retry it later then [11:19] mardy sadly spread rarely passes in snapd [11:19] as tests are a bit too integration testing sometimes [11:19] and picking up all kinds of motion in the ecosystem [11:19] distros and the store alike [11:37] pedronis: any idea how I could test the snapId check? With the latest changes, all local snaps can be installed (since we use --dangerous), so I'm struggling to find a way to test this: https://github.com/anonymouse64/snapd/blob/feature/snap-microk8s-shared-system-usernames/tests/main/system-usernames-microk8s/task.yaml#L7-L13 [11:38] mardy: you need to involve the fake store I fear to test this, otherwise you'll have no snap-ids [11:39] pedronis: ah, I didn't know we had a fake store for the tests! Sounds cool :-) [11:39] mardy: I think we have a similar case already, let me check [11:41] mardy: devicestate.CanManageRefreshes is similar in the sense it has a check that depends on the snap having a snap-id [11:42] mardy: so I think the spread test for it should have at least some inspiration: ./tests/main/interfaces-snapd-control-with-manage/task.yaml [11:43] mborzecki: I made some high-level comments in https://github.com/snapcore/snapd/pull/10436 [11:43] thanks [11:49] mborzecki: added another one [12:17] pstolowski: I think we forgot to consider something related to error handling: https://github.com/snapcore/snapd/pull/10408#discussion_r657036500 [12:17] unless I'm confused [12:21] pedronis: dang, you are right, runHook will call our handler but then error the task [12:22] pedronis: ok as a followup? [12:23] pstolowski: yes, but we need to decide a bit what to do there [12:26] pstolowski: I think we have very few hooks (handlers) fwiw that have non-trivial Error behavior [12:30] pedronis: maybe even none? [12:30] pstolowski: healthstate I think does [12:31] indeed [12:40] pstolowski: should we chat quickly about this now or after the standup? [12:58] pedronis: sorry, was a bit distracted, maybe after standup then? [13:01] pstolowski: yes [14:52] mardy: i added a general comment https://github.com/snapcore/snapd/pull/10438/files#r657180723, hope it makes sense [15:00] pstolowski: ah! Didn't think of that! Thanks, will find another way.