Merge lp://qastaging/~sil2100/ubuntu-release-upgrader/snap-size-estimation into lp://qastaging/ubuntu-release-upgrader
Status: | Merged |
---|---|
Merged at revision: | 3280 |
Proposed branch: | lp://qastaging/~sil2100/ubuntu-release-upgrader/snap-size-estimation |
Merge into: | lp://qastaging/ubuntu-release-upgrader |
Diff against target: |
440 lines (+321/-38) 3 files modified
DistUpgrade/DistUpgradeCache.py (+2/-0) DistUpgrade/DistUpgradeQuirks.py (+115/-38) tests/test_quirks.py (+204/-0) |
To merge this branch: | bzr merge lp://qastaging/~sil2100/ubuntu-release-upgrader/snap-size-estimation |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Brian Murray | Needs Fixing | ||
Review via email:
|
Commit message
Modify DistUpgradeQuirks to calculate extra space needed for snaps that need to be installed during upgrade (replacing old debs) and add that to estimates done in checkFreeSpace().
Description of the change
Modify DistUpgradeQuirks to calculate extra space needed for snaps that need to be installed during upgrade (replacing old debs) and add that to estimates done in checkFreeSpace().
This branch is now ready for an initial review. I had a few design bikeshedding issues while working on this, so I'm still not entirely happy with the end result. For instance, I don't like how it looks with the consecutive quirks assuming the previous ones set up everything for them - but that seemed to be optimal.
I added all the estimation as a quirk, alongside of the existing ones for replacing debs with snaps. Seemed to be 'consistent' with the current approach. In cases where quirks are not ran, the extra_snap_space is 0 so nothing funny is happening. I also added some very basic unit testing for the new methods added. They're not very pretty, mostly because a lot of the functionality had to be mocked as it required network access, which we don't want to rely on in unit testing.
For fetching the size, I try to be as accurate as possible - which is why I am performing raw API calls to the snapstore as per their recommendation. I could not use snap_info as we need to get the size of the snap for the specific branch (stable/ubuntu-x.x) - and only a fake snap_refresh allows that (which needs the snap-id specified). At first I wanted to be consistent and just use `snap info`, but as said - the command didn't seem to allow specifying the branch that needed to be checked.
I'm open to any propositions. This is my first contribution to u-r-u so apologies if the coding style is not adequate. I still need to test it in practice, which is why I'd prefer if we didn't merge it yet before more testing.
From the stylistic nitpicks for instance, I really had a strong urge to just call _prepare_ snap_replacemen t_data( ) in both _calculateSnapS izeRequirements () and _replaceDebsWit hSnaps( ), and simply make it to exit early if self._snap_list is already non-empty. But then it would be non-optimal, since we know _replaceDebsWit hSnaps will never be called without _calculateSnapS izeRequirements running first. But eh, I somehow couldn't make this look pretty, sorry...