Code review comment for lp://qastaging/~sil2100/ubuntu-release-upgrader/snap-size-estimation

Thanks for the review! I will address all the issues you have outlined, with comments to a few of them (not including those inline as the diff will change and it's always a bit more annoying to switch to the old revision to see the comments):

1) Regarding PreCalcDistUpgrade: actually I see that I can just do the same in PostInitialUpdate, not sure why those weeks ago I didn't find that one appropriate. I just know any of the later ones would not do, as it has to happen early in the upgrade process. One plus side of having a PreCalcDistUpgrade was that it all felt a bit more consistent, as the size calculation was done right before the dist-upgrade was 'calculated'. I changed it to be done as part of PostInitialUpdate now which is also fine, but the only downside is that now very early users might be seeing 'Calculating snap size requirements', which might seem a bit out-of-place. On the other hand, that doesn't actually matter much I'd say!

2) Regarding changing the 'installing ...' strings as you requested - won't that break translations? I mean, I'm usually very reluctant in changing translatable strings without a valid reason. Guess this particular change won't be backported to bionic though, so maybe it's actually safe. Anyway, I guess I'd recommend changing those in a separate MP/commit.

3) Regarding Snap-Device-Series: for now this is a constant that I think can just stay hard-coded. One might think it's related to the core base (16, 18 etc.) - that would be logical, right? But actually it's not. I was confused about that as well, but the snapd team told me this series is actually unrelated to the snap base but is instead like a snap-specific standard version which should be only bumped when the standard changes, which for now is still constant. So this argument is '16' for both core16, core18 and possibly the upcoming core20.

4) Regarding the count of 5 in the test for replacing debs with snaps: I guess it feels correct, because basically currently as well as previously when determining which debs need to be exchanged with snaps, at the stage of when the quirk is running there was never any logic to check if we're replacing a real deb or not. So for instance 'gtk-common-themes', which is not a real deb package, in the case when its snap was not installed on the system, would anyway mark it for installation and put it into forced_obsoletes. The logic here didn't change. What the unit test checks is only if the 'refresh' snaps are refreshed and if the 'install' snaps are installed (and, per the existing logic, added to the forced_obsoletes).
I can only assume that the upgrader actually checks if the packages in forced_obsoletes are actual packages that need acting on somewhere later on.

5) Regarding the new dependencies and adding those to debian/control: I think no new dependencies need to be added. Both the json and urllib modules are part of the python standard library, so they basically come from the libpython3.*-stdlib etc. packages, and those are explicit dependencies of python3.*. So I guess we should be safe here, right?