Code review comment for lp:~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!

Great, thanks.

> 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.

That's fair and not a big deal really.

> 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.

Alright, thanks for explaining this!

> 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.

That makes sense, maybe its worth adding a comment about why it is 5 though?

> 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?

Yes, that's true I didn't look at where the modules come from.