| mborzecki | morning | 06:10 |
|---|---|---|
| zyga | good morning | 06:27 |
| mborzecki | zyga: hey | 06:36 |
| mborzecki | zyga: miserable weather, grey, dull, raining a bit :/ | 06:38 |
| zyga | mborzecki finally! :D | 06:51 |
| zyga | mborzecki it was sunny for too long | 06:51 |
| pstolowski | morning | 07:00 |
| pedronis | pstolowski: hi | 07:00 |
| zyga | good morning pawel | 07:01 |
| zyga | pstolowski could you look at the fruit of your recent reviews please? | 07:01 |
| zyga | pstolowski https://github.com/snapcore/snapd/pull/9446 ! :) | 07:02 |
| pstolowski | zyga: hey, nice! i saw your tweet about it :) | 07:02 |
| mvo | hey pstolowski and zyga | 07:02 |
| pstolowski | will look at it for sure | 07:02 |
| zyga | hey mvo :) | 07:02 |
| zyga | mvo I will focus on the feedback I got from samuele today | 07:02 |
| zyga | mvo I would love if you could look at https://github.com/snapcore/snapd/pull/9446 | 07:02 |
| zyga | mvo it can still be in edge before the sprint :) | 07:03 |
| mvo | zyga: ok, let me look | 07:03 |
| zyga | mvo it's mostly questions and some code | 07:06 |
| zyga | but it could be in edge by Friday ;-) | 07:06 |
| zyga | just saying | 07:06 |
| pedronis | mvo: docker-smoke is failing on 20.10, failure seems real | 07:07 |
| pedronis | mvo: https://github.com/snapcore/snapd/pull/9155/checks?check_run_id=1189677047 | 07:08 |
| mvo | pedronis: uh, thanks, I have not seen this, will also look | 07:08 |
| pedronis | might be aa3 related | 07:08 |
| zyga | strconv.Atoi: parsing "file": invalid syntax. | 07:09 |
| zyga | weird | 07:09 |
| zyga | we try to parse "file" as int | 07:09 |
| zyga | whatever that is | 07:09 |
| pedronis | degville: mvo: different note, could you double check https://github.com/snapcore/snapd/pull/9424 looks | 07:09 |
| pedronis | zyga: not sure it's us, it seems docker is trying to do that | 07:10 |
| zyga | ah, I see | 07:10 |
| pedronis | ... 9424 looks good | 07:10 |
| pedronis | pstolowski: could you look at my last commit in #9155 , I changed something ? | 07:11 |
| pstolowski | pedronis: sure | 07:13 |
| mvo | pedronis: sure | 07:15 |
| degville | pedronis: looks good! good catch on the blank line. | 07:16 |
| mvo | pedronis: looks good (9424) - do you mind if I squash merge? then I will cherry pick into 2.47 | 07:17 |
| pedronis | mvo: yea, it's fine, thanks | 07:18 |
| mvo | pedronis: thanks, merged and cherry picked | 07:21 |
| mvo | pedronis: docker failure looks real indeed, running a spread -debug now to see what is going on | 07:21 |
| mvo | zyga: 9446 is just 200 LOC ? woah | 07:22 |
| zyga | mvo: it's an early draft, but yeah | 07:22 |
| mborzecki | mvo: can you take a look at https://github.com/snapcore/snapd/pull/9439 ? it's super simple | 07:26 |
| mvo | zyga: first read - very very nice | 07:28 |
| zyga | mvo \o/ | 07:28 |
| mvo | mborzecki: sure | 07:28 |
| zyga | man those reviews make all the difference in world | 07:28 |
| zyga | lately all my code was "after core20" :-) | 07:28 |
| mborzecki | mvo: thanks! | 07:29 |
| mvo | zyga: but we are past this now - you are IMPORTANT again ;) | 07:31 |
| mvo | mborzecki: hm, I saw two failures of "snap sign" on arch, wonder if something changed there | 07:32 |
| * zyga installs macos update, afk for coffee | 07:32 | |
| mvo | mborzecki: merged it anyway, I don't think your code broke this ;) | 07:32 |
| mborzecki | mvo: thanks, let me check the log | 07:32 |
| mborzecki | mvo: hmm, looks like another gpg problem | 07:33 |
| pedronis | we need to spend some time understanding those problems, they are more frequent, and we never spent time understanding them | 07:41 |
| pedronis | nothing immediate though | 07:41 |
| pedronis | mborzecki: I reviewed #9440, it looks good, but I'm confused by how it keeps things working alone | 07:42 |
| pedronis | (probably my confusion) | 07:42 |
| pedronis | mborzecki: interesting that no unit test broke? | 07:45 |
| mborzecki | pedronis: you're right about that snippet, need to fix that | 07:45 |
| pedronis | pstolowski: thanks for the review | 07:45 |
| mborzecki | pedronis: also, the observer does not respond with ChangeIgnore atm yet, so we're still keeping the behavior where managed assets are asked upfront when creating the updater | 07:46 |
| pedronis | mborzecki: yea | 07:47 |
| mborzecki | pedronis: new behavior is in #9427 | 07:48 |
| mborzecki | hm mup didn't pick that up | 07:48 |
| mborzecki | mup: poke poke | 07:48 |
| mvo | pedronis: re docker> I suspect this happens because 20.10 has the new apparmor3 but the core snap has apparmor_parser2. i.e. the apparmor_parser(v2) from the core snap tries to read new apparmor.conf and does not understand it :/ | 07:49 |
| mborzecki | looks like it needs a poke from niemeyer | 07:49 |
| zyga | mvo oooh | 07:56 |
| zyga | mvo very problematic actually | 08:02 |
| mvo | zyga: yeah, makes my head hurt :( | 08:02 |
| mvo | zyga: I think we need to talk to security jamie about this | 08:02 |
| pedronis | mvo: all sounded too easy | 08:02 |
| zyga | yes | 08:02 |
| zyga | it may seem we need apparmor 3 in core then? | 08:02 |
| * zyga reboots | 08:02 | |
| pedronis | core18 would have the same issue | 08:02 |
| pedronis | ? | 08:03 |
| pstolowski | pedronis: updated #9442 | 08:09 |
| mvo | pstolowski: \o/ | 08:40 |
| zyga | jamesh reviewed | 08:46 |
| zyga | jamesh my main question there is the way we ask the store | 08:46 |
| zyga | is this agreed with pedronis? | 08:46 |
| jamesh | zyga: the code in the PR is a translation of some stuff I was doing client side. There was some discussion on the forum about how to convert theme names to package names with some community feedback, but I don't think there was anything from the snapd side. | 08:48 |
| jamesh | zyga: part of the idea was to keep the API relatively high level so it could be adapted to newer store query APIs if they become available | 08:49 |
| jamesh | (e.g. a search query based on slot definitions) | 08:50 |
| zyga | I understand that API may be still far away, I'm mostly concerned about suitability of the implementation in the meanwhile, will the store load be acceptable? | 08:50 |
| zyga | if that's a yes, then I have no objections | 08:50 |
| jamesh | zyga: I expect that for the majority of users it will result in zero load on the store | 08:51 |
| mborzecki | pedronis: updated https://github.com/snapcore/snapd/pull/9440 now | 08:51 |
| jamesh | zyga: most users' themes are provided by gtk-common-themes, so a store lookup would be avoided | 08:51 |
| zyga | mmm, | 08:52 |
| zyga | jamesh what will trigger the search? I understand this is a new API that's not used yet | 08:52 |
| zyga | jamesh should we add provisions for avoiding searching on failures, e.g. to cap them at once a day or something | 08:52 |
| jamesh | zyga: a daemon running in the user session. It will do one check on login, and any time the theme settings are altered | 08:53 |
| zyga | jamesh ok | 08:53 |
| zyga | I think we can introduce any kind of caching and rate-limiting there | 08:53 |
| zyga | anyway, have a look at the rest of the review, I think this was my main concern | 08:54 |
| zyga | the rest is trivial | 08:54 |
| jamesh | thank you for going over it | 08:54 |
| zyga | my pleasure, all I need is a ping when my manager is looking at me ;-) | 09:01 |
| pedronis | jamesh: is it using info or find logic ? | 09:25 |
| zyga | pedronis IIRC info | 09:28 |
| jamesh | pedronis: it does full name based queries rather than pattern matching | 09:28 |
| pedronis | it should be ok (at least for a first pass), but yes we could add caching/rate limit, or discuss with the store | 09:37 |
| pedronis | otoh we should look into using the functionality that let us ask only for fields we really need | 09:37 |
| mvo | mwhudson: hey, is stripping go binaries safe these days? I saw some old discussions about this in 2016 and was wondering if there is anything new happening since? would be nice for snapd :) | 09:41 |
| mwhudson | mvo: should be fine yes | 09:41 |
| zyga | mwhudson: can we strip the line table? | 09:41 |
| pedronis | pstolowski: in a follow we can teach Iter to take an optional SetID and use that it list, so we stop reading bits of snapshots to find the files of one | 09:41 |
| zyga | or anything else that is somewhat optional and inflates the binary size | 09:42 |
| mvo | mwhudson: nice, thank you | 09:42 |
| mwhudson | zyga: not sure, do you care about go tracebacks? | 09:42 |
| zyga | mwhudson not so much in that context | 09:42 |
| zyga | mwhudson: to be clear, size above all nice extras | 09:43 |
| mwhudson | zyga: can't actually remember what's in .gopclntab, it's been a while :) | 09:43 |
| pstolowski | pedronis: yes, indeed | 09:43 |
| mwhudson | some of the mapping tables are important | 09:43 |
| pedronis | zyga: heh, I would say more measure and decide, we do care about fixing bugs and traceback might be useful | 09:46 |
| zyga | pedronis: we did measure, we are larger than the rest of the OS, we should figure out what we _can_ strip if possible | 09:47 |
| zyga | I agree in priciple but it's just "can we make go smaller" kind of thing | 09:47 |
| pedronis | #9234 btw needs 2nd reviews | 09:59 |
| pedronis | https://github.com/snapcore/snapd/pull/9234 | 09:59 |
| pedronis | pstolowski: what's the status of #8395 ? | 10:01 |
| pstolowski | pedronis: hmm a bit forgotten tbh, let me merge master and see | 10:02 |
| pedronis | pstolowski: mvo said some tests were failing, anyway not urgent but it would be good to land it before 2.49 | 10:03 |
| pstolowski | pedronis: yeah i'm going to check what's failing | 10:03 |
| pstolowski | ah just gomt i think | 10:04 |
| pstolowski | *gofmt | 10:04 |
| pedronis | pstolowski: I merged my validation sets PR btw | 10:09 |
| pstolowski | ack, ty | 10:11 |
| * zyga would love a front-end for strace logs that can offer either time-correlated or independent view of various processes | 10:36 | |
| zyga | one day, I will learn GUI programming and write that | 10:36 |
| pedronis | pstolowski: #9442 can land I think? pstolowski, mvo: can we update #9036 with master then to use the changes pstolowski did? | 10:36 |
| pstolowski | ok merging, was waiting for tests | 10:38 |
| mvo | pedronis: sure thing! | 10:40 |
| mvo | pstolowski: should I take care of 9036 (merge master, deconflict?) | 10:46 |
| pstolowski | mvo: i can do it | 10:48 |
| mvo | pstolowski: either way is fine with me | 10:49 |
| mvo | pstolowski: so please go ahead, I will review once it's there. I added some review points last evening too that I failed to get to so far :/ | 10:49 |
| pstolowski | mvo: ok | 10:49 |
| mborzecki | anyone familiar with aliases in snap declarations? | 11:04 |
| mborzecki | is this correct? https://paste.ubuntu.com/p/9ZrpDYKGf9/ shouldn't the target be dotnet-sdk.dotnet? | 11:05 |
| mborzecki | pedronis: maybe you know ^^ this is from https://bugs.launchpad.net/snapd/+bug/1897984 | 11:05 |
| mborzecki | hm looking at the code the auto alias should be should be dotnet:dotnet i think | 11:07 |
| zyga-mbp | mborzecki perhaps the snap name is implicit | 11:08 |
| pstolowski | mvo: i've pushed just deconflict + trivial fix, will simplify now | 11:08 |
| mborzecki | zyga-mbp: hm the code looks for info.Apps[<alias-target>], maybe they had dotnet-sdk at some point, but there's no dotnet app | 11:09 |
| mvo | pstolowski: nice | 11:09 |
| mborzecki | zyga-mbp: i mean there's no dotnet-sdk app at this point, maybe it was there before | 11:09 |
| pedronis | mborzecki: there shouldn't be a dot there | 11:32 |
| mborzecki | pedronis: so name: dotnet, target: dotnet (as the app name is `dotnet`) | 11:33 |
| pedronis | yes | 11:34 |
| mborzecki | ack, left a note in the bug | 11:34 |
| pedronis | maybe we should add some validation code | 11:34 |
| pedronis | for this | 11:34 |
| mborzecki | pedronis: or a log at least, that the the auto alias does not reference a known app | 11:35 |
| pedronis | no, I mean in the assertion parsing | 11:35 |
| pedronis | mborzecki: sorry, there not dot problem, but they are pointing dotnet -> donet-sdk | 11:39 |
| pedronis | instead of dotnet -> dotnet | 11:40 |
| mborzecki | pedronis: so this basically right? https://bugs.launchpad.net/snapd/+bug/1897984/comments/1 | 11:40 |
| pedronis | yes | 11:41 |
| mborzecki | cool, thanks for double checking | 11:41 |
| pedronis | this is a bit messy though, if they really had a dotnet-sdk before and not now | 11:42 |
| pedronis | we can't make it work both ways | 11:42 |
| mborzecki | cmatsuoka: hi, can you take a look at https://github.com/snapcore/snapd/pull/9440 ? | 11:45 |
| cmatsuoka | mborzecki: sure, checking it | 11:45 |
| mborzecki | cmatsuoka: thanks! | 11:45 |
| jdstrand | mvo: re aa3> can you discuss with amurray? we have on our roadmap to vendor aa3 next cycle. also note, I've been running aa3 with snaps for weeks and weeks (but I don't use the docker snap). if it is an apparmor2 vs apparmor3 config file issue, the easiest thing would be to ship apparmor.conf from aa2 and use --config-file | 11:59 |
| jdstrand | mvo: jjohansen would also be able to help triage | 11:59 |
| jdstrand | mvo: that said, I don't see where snapd is shipping apparmor_parser so it should just use whatever is on the system and the system should have everything it needs? | 12:01 |
| jdstrand | oh, or is it the docker snap that is shipping it... | 12:01 |
| pedronis | well, or using it from the base snap | 12:01 |
| pedronis | the base snap does have it (for core) | 12:01 |
| pedronis | but it would need interface support for that | 12:02 |
| pedronis | jdstrand: so we have to meditate on this: | 12:02 |
| pedronis | docker_support.go:/sbin/apparmor_parser ixr, | 12:02 |
| pedronis | multipass_support.go:/sbin/apparmor_parser ixr, | 12:03 |
| jdstrand | multipass-support and docker-support... yes | 12:03 |
| pedronis | it's also probably related to etc comes straight from host | 12:04 |
| jdstrand | yes | 12:04 |
| * jdstrand notices that core20 has apparmor_parser in /usr/sbin and those interfaces are broken | 12:05 | |
| jdstrand | ok | 12:07 |
| jdstrand | this appears non-fatal | 12:07 |
| jdstrand | well, it depends on the core | 12:08 |
| pedronis | jdstrand: would the user land v2 parser even work if not choking on the config with the rest of the system using aa3 ? | 12:09 |
| jdstrand | 'yes', if it can understand the policy | 12:10 |
| jdstrand | gimme a sec | 12:10 |
| pedronis | mborzecki: I fixed the dotnet alias, can you try and comment in the places? afaict all the recent release revisons have dotnet as app, not dotnet-sdk | 12:10 |
| mborzecki | pedronis: sure | 12:11 |
| mborzecki | pedronis: thanks, it work snow, the alias was created | 12:12 |
| pedronis | mborzecki: if they had current versions around with app dotnet-sdk and some with dotnet, atm we could have made only one of the two work | 12:13 |
| pedronis | but if people are updated to what is on the channels it should work | 12:13 |
| pedronis | /on the/in the/ | 12:14 |
| zyga_ | while this is not a solution, I really really think we need to plan a way out of shared /etc | 12:14 |
| zyga_ | it was a huge mistake, we should only share specific files, perhaps with a filter even | 12:14 |
| === zyga_ is now known as zyga | ||
| pedronis | zyga: yes, I would like to do that, but at this point is a 22 goal | 12:15 |
| pedronis | (and maybe even that is optimistic) | 12:15 |
| zyga | as long as it's on the roadmap somewhere, I'm happy | 12:15 |
| zyga | well | 12:15 |
| zyga | I think we may have our hand forced at some point | 12:16 |
| jdstrand | pedronis and zyga (cc mvo and amurray): https://paste.ubuntu.com/p/d82JTRMdxz/ | 12:21 |
| jdstrand | I'm going to file a bug | 12:22 |
| jdstrand | pedronis and zyga (cc mvo and amurray): https://bugs.launchpad.net/snapd/+bug/1898038. I've assigned amurray for the moment, but if someone else is going to jump on it, please let him know | 12:27 |
| * jdstrand adds a 20.10 Ubuntu task too | 12:27 | |
| zyga | thanks | 12:29 |
| jdstrand | stgraber: you might be interested in that bug too ^ (that said, I've been using lxd with apparmor3 for weeks and weeks and you seem ok) | 12:29 |
| jdstrand | amurray: we should update our test plan to include docker, lxd and multipass snaps | 12:32 |
| pedronis | zyga: I commented on #7700 | 12:33 |
| cmatsuoka | mborzecki: done | 12:34 |
| mborzecki | cmatsuoka: thanks, let me land that and update the main branch | 12:34 |
| pedronis | pstolowski: mvo: should I look myself at #9036 (snapshot import) or is it premature? | 12:35 |
| pstolowski | pedronis: 1 minute, i'm just finishing actual changes | 12:36 |
| pstolowski | so far only pushed deconflicting changes | 12:36 |
| jdstrand | amurray: note, snap-confine has facilities for this sort of thing, but the core snaps don't ship /etc/apparmor and /etc/apparmor.d currently, so they'll need to be adjust to not strip those out | 12:38 |
| * jdstrand steps away for a bit | 12:38 | |
| pstolowski | pedronis: i pushed simplifications after changes in master. but this PR still needs more work to address all comments | 12:39 |
| * zyga lunch | 12:40 | |
| zyga | pedronis thank you | 12:42 |
| pedronis | pstolowski: made some small comments | 12:47 |
| mvo | pedronis: thanks! how do you feel about it overall? | 12:49 |
| pedronis | mvo: I don't know, I mostly looked at backend | 12:50 |
| mvo | ok | 12:52 |
| * pstolowski lunch | 13:56 | |
| pedronis | mborzecki: I did a pass on #9443 | 14:16 |
| mborzecki | pedronis: thanks! | 14:18 |
| pedronis | pstolowski: how annoying would be to split the import PR into two ? (overlord/snapshotstate and the rest) it's not very big but it touches a lot files | 14:22 |
| mvo | pedronis, pstolowski I can do that if it helps you, I can do the api bits maybe first? those look ok | 14:26 |
| mvo | (to me9 | 14:26 |
| pedronis | it would help a little bit my reviewing | 14:30 |
| mvo | pedronis: sure, let me do this now, we may lose a bit of history unless I cherry pick stuff which will be super messy | 14:34 |
| jdstrand | Saviq: fyi, you are likely affected by https://bugs.launchpad.net/snapd/+bug/1898038. see backscroll starting ~2.5 hours ago | 14:49 |
| pstolowski | mvo, pedronis sorry, had a late lunch. sounds good. i'll focus on addressing the comments to backend part | 14:50 |
| mvo | pstolowski: thank you! | 14:51 |
| mvo | zyga: do you need anything from me? | 14:51 |
| mvo | pedronis: I pushed 9450 | 14:51 |
| zyga | mvo: a review on the notification thing would be great | 14:51 |
| zyga | mvo: nothing else from the top of my head | 14:51 |
| mvo | zyga: the one that actually sents the notifications (i.e. the latest one?) | 14:52 |
| zyga | yes | 14:52 |
| zyga | cachio I did a pass through most or all of your PRs | 14:58 |
| cachio | zyga, thanks!! | 14:58 |
| cachio | I'll take a look now | 14:58 |
| ijohnson | hey folks | 14:59 |
| * ijohnson is back finally | 14:59 | |
| ijohnson | mvo: jdstrand: sounds like docker on groovy stuff got sorted out and triaged at least ? | 15:01 |
| zyga | ijohnson: I think we have a life jacket at least | 15:01 |
| ijohnson | that's nice | 15:01 |
| ijohnson | btw do layouts to put things from $SNAP onto /etc/... work ? | 15:02 |
| zyga | ijohnson we should plan something more oomhpy over time and there are some proposals (bundle aa with snapd, stop using host's /etc, fix immediately with mount profiles when aa-capable interfaces are connected) | 15:02 |
| ijohnson | last time I tried that it didn't work because snap-update-ns died saying that trying to do the specified layout would modify the host | 15:02 |
| ijohnson | but maybe I was doing something else wrong | 15:02 |
| zyga | ijohnson: we can do better, each interface that can use apparmor_parser could result in hiding real /etc/apparmor* | 15:02 |
| ijohnson | zyga: yes that makes sense | 15:02 |
| * zyga stops coding now, stops reviewing, need to take a break and work with the kids a little | 15:03 | |
| zyga | I'll be back late to iterate on my own PRs a little | 15:03 |
| jdstrand | ijohnson: layouts were designed to handle /etc... the idea is that the snap could ship its own /etc/apparmor and /etc/apparmor.d and then have a layout for it | 15:03 |
| jdstrand | ijohnson: that is the escape hatch to unbreak things without snapd changes. snapd can do better though. and yes, it is triaged | 15:04 |
| jdstrand | (and assigned) | 15:04 |
| jdstrand | ijohnson: hello btw :) | 15:04 |
| * jdstrand wouldn't mind jumping on this himself... | 15:05 | |
| ijohnson | hi :-) | 15:05 |
| jdstrand | </sniff> | 15:05 |
| zyga | jdstrand what do you think about the idea I mentioned? | 15:05 |
| ijohnson | yeah I udnerstand that layouts are supposed to work on /etc, I just seem to remember some problem with them on /etc but perhaps I am mistaken | 15:05 |
| zyga | ijohnson it IS a problem because etc is unpredictable | 15:05 |
| zyga | if a distribution provides /etc/apparmor -> /etc/apparmor-2.0 symlink | 15:05 |
| zyga | we are broken | 15:05 |
| zyga | it's not a nice solution to force this on snap authors IMO | 15:06 |
| jdstrand | no | 15:06 |
| jdstrand | that isn't the solution | 15:06 |
| ijohnson | zyga: ah right I think that was what I hit, is that something on my host /etc/ was interfering with setting up the mimic | 15:06 |
| jdstrand | that is just if the publisher insists on unbreaking themselves before the solution is in place | 15:06 |
| zyga | ah, I misunderstood then | 15:06 |
| zyga | yes, the publisher has an escape hatch as well | 15:07 |
| jdstrand | zyga: doing it in the interface could be nice since only two interfaces need this behavior. the interface code also understands what base is in use and can mount the correct one | 15:08 |
| zyga | yeah | 15:08 |
| jdstrand | zyga: perhaps add a comment to the bug how you think this might work for amurray? | 15:08 |
| zyga | and it's more open than a layout (which is by design more constrained) | 15:08 |
| zyga | jdstrand I did :) | 15:08 |
| jdstrand | ah | 15:09 |
| * jdstrand reads | 15:09 | |
| zyga | well, terse but so :) | 15:09 |
| * zyga EPDs | 15:15 | |
| zyga | EODs even | 15:17 |
| * cachio lunch | 15:17 | |
| jdstrand | zyga: ok, commented in the bug. enjoy your evening | 15:27 |
| pedronis | mvo: I reviewed #9450 | 15:45 |
| mvo | pedronis: thanks! working through it right now | 15:49 |
| mvo | pstolowski: please push any WIP for the import pr before you EOD, I can pick up from that then :) | 15:49 |
| pstolowski | mvo: ok, i addressed the couple of minor points, but the big one is comparing hashes, pondering about that now | 15:52 |
| pstolowski | s/the/a/ | 15:52 |
| pedronis | pstolowski: to be clear, that probanly can be/should be its own PR | 15:54 |
| pedronis | but I didn't want us to forget about it | 15:55 |
| pstolowski | pedronis: ah, ok, thanks | 15:55 |
| pedronis | at least you can put a TODO in that fuction? is it even the place where we would do this? | 15:55 |
| mvo | pstolowski, pedronis thanks, +1 for followup | 16:00 |
| mvo | pedronis: looking at /v2/snapshots right now, I think that's what I will do | 16:01 |
| pstolowski | mvo: pedronis do we want to verify hashes at // XXX: check the snapshot hashes or in a followupt oo | 16:01 |
| pstolowski | *too | 16:01 |
| pedronis | pstolowski: that sounds like something we should to be sure the thing wasn't corrupted? | 16:02 |
| pstolowski | pedronis: indeed, mostly asking if we want it in this PR or followup | 16:03 |
| pedronis | pstolowski: we have code for check-snapshot, maybe some part of it can be reused | 16:03 |
| pedronis | not sure | 16:03 |
| pstolowski | yes | 16:03 |
| pstolowski | checkOne | 16:03 |
| pstolowski | actually, Check | 16:03 |
| mvo | pedronis: do you have a recommendation for the content-type? application/snapd-snapshot-v1 ? or too outlandish? versioning seems to be uncommon :/ | 16:05 |
| pstolowski | it would helped if Iter() wasn't assuming dirs.SnapshotsDir but again it probably should be a prerequisite (down a rabbit hole ;)) | 16:08 |
| pstolowski | hmm, maybe not neccessary | 16:11 |
| mvo | pedronis: thanks, just saw your updated review! I will address after I made dinner for the kids | 16:13 |
| mvo | pedronis: I pushed a stawman for the content-type too now please comment in the PR :) | 16:13 |
| pedronis | mvo: yea, I looked at what save does | 16:13 |
| * mvo needs to vanish for ~45min or so | 16:13 | |
| pedronis | done | 16:18 |
| pstolowski | mvo: pushed a few more changes | 17:11 |
| pstolowski | mvo: added Check() and a very rudimentary test for it.. would love to have a good test, but it's tricky. i may take a look at it tomorrow with clear head | 17:13 |
| pedronis | mvo: I changed my thinking slightly: https://github.com/snapcore/snapd/pull/9450#discussion_r498399310 to be consistent with the design as it was discussed | 17:17 |
| mvo | pedronis: yeah, noticed and update | 17:51 |
| pedronis | mvo: I made another comment (but maybe I'm confused), anyway I don't think #9450 needs my +1 at this point, my main concern were addressed, but I don't think I'm up to do a final close review right now | 18:04 |
| pedronis | if I'm making sense | 18:08 |
| mvo | pedronis: sure, that's fine | 18:25 |
| mvo | pedronis: I will ask the others tomorrow to review the details, thanks a bunch! | 18:25 |
| ijohnson | pedronis: got a second for quick question on how to set a client.Client config option for cmd/snap ? I want to specify a Timeout to the http.Client we use for our snapd client.Client, because currently there is not one, but I only need it for the console-conf routine command, not for every command in snap | 18:32 |
| ijohnson | pedronis: I was thinking about just tearing out the existing client from my Execute() and recreating a client, but that seems fragile and wrong | 18:32 |
| ijohnson | or maybe it's okay to have a reasonable Timeout for all snap commands | 18:34 |
| pedronis | ijohnson: there is kind of a timeout for all client requests, no? | 19:12 |
| ijohnson | pedronis I don't see an http.Client Timeout set for the client that we use from clientMixIn | 19:13 |
| ijohnson | Maybe I'm reading the wrong code | 19:13 |
| pedronis | ijohnson: no, but client itself sets up a timeout via context, for everything except things that use client.raw directly | 19:14 |
| ijohnson | Ah right I forgot about contexts timeout | 19:14 |
| * ijohnson goes to go look at that | 19:15 | |
| pedronis | do uses doTimeout which goes into rawWithTimeout | 19:15 |
| pedronis | I mean client.do | 19:15 |
| pedronis | Client.do | 19:15 |
| pedronis | in client/client.go | 19:15 |
| ijohnson | pedronis yes I see that now | 19:16 |
| ijohnson | I was just looking at client.Config | 19:16 |
| pedronis | it's fixed | 19:17 |
| pedronis | except some things mostly dealing with downloading blobs from snapd don't use it | 19:17 |
| pedronis | downloading or uploading | 19:17 |
| ijohnson | Okay I think I just need to modify my call to Do() to use a different shorter timeout then | 19:18 |
| ijohnson | Probably I could just call rawWithTimeout | 19:19 |
| pedronis | yes, most of the code in do seems only relevant for GET, plus the decoding but that is an helper as well, so you could call it yourself | 19:22 |
| pedronis | but doSync does a bit more | 19:23 |
| pedronis | this code honestly seems to need a cleanup, I think it was meant to be very standardized | 19:24 |
| pedronis | but now we have grown different variant, and there's a bit of mix between params vs variants | 19:24 |
| pedronis | to ask for different behavior | 19:24 |
| ijohnson | yes there's a few different ways to do subtly different versions of the same thing it seems | 19:29 |
| pedronis | yes | 19:30 |
| pedronis | like NoTimeout is weird | 19:32 |
| ijohnson | I'm beginning to wonder if there's any part of our codebase that hasn't grown organically like this | 19:33 |
| pedronis | it's natural, but is also necessary to clean up once in a while | 19:34 |
| pedronis | this one seems particularly bad though | 19:34 |
| pedronis | I don't think it has deep reasons to be like | 19:34 |
| pedronis | except to avoid changing call points | 19:34 |
| pedronis | but all call points are inside client anyway | 19:34 |
| ijohnson | Right | 19:36 |
| pedronis | it's even worse, some places do raw but set their own timeout | 19:41 |
| pedronis | :/ | 19:41 |
| ijohnson | :-( | 19:46 |
| pedronis | I thing they preexist rawWithTimeout but anyway :/ | 19:54 |
| pedronis | ijohnson: I cleaned it up a bit | 20:35 |
| ijohnson | pedronis thanks for that | 20:37 |
| pedronis | ijohnson: you were doing doSync right? | 20:38 |
| ijohnson | pedronis yes I was using doSync originally | 20:39 |
| pedronis | yea, now there's a doSync variant that can be passed a specific timeout | 20:39 |
| ijohnson | Great thanks | 20:40 |
| ijohnson | mup seems to be mia | 20:40 |
| pedronis | ijohnson: here: https://github.com/snapcore/snapd/pull/9454 if you could look at it and maybe pick it up tomorrow, it could use some tests, it's mostly a refactor but there are some changes to do, tbh lots of the new stuff didn't seem tested yet | 20:44 |
| ijohnson | pedronis: sure I am going to work a bit extra tonight to make up for starting late today so I can try to clean that up | 20:44 |
| ijohnson | also hoping to get my first pr for the maintenance.json stuff up that is related to here | 20:45 |
| pedronis | maybe that was confusing: it's mostly a refactor but there are some changes to the "do" method | 20:59 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!