Merge ubuntu-cdimage:lp.2063471 into ubuntu-cdimage:main

Proposed by Steve Langasek
Status: Merged
Approved by: Łukasz Zemczak
Approved revision: a6e6f40492f5f9a0a413cff6e42814bfee6bf1c7
Merged at revision: a6e6f40492f5f9a0a413cff6e42814bfee6bf1c7
Proposed branch: ubuntu-cdimage:lp.2063471
Merge into: ubuntu-cdimage:main
Diff against target: 13 lines (+1/-1)
1 file modified
lib/cdimage/checksums.py (+1/-1)
Reviewer Review Type Date Requested Status
Łukasz Zemczak Approve
Review via email: mp+465012@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Łukasz Zemczak (sil2100) wrote :

This will work and is certainly not harmful, but I'm a bit against doing such very specific special-casing in the main checksum code. Are we certain that the only cases we will see for netboot are the ones requiring re-packing of the tarball? Looking at the code... it seems so. If you checked that that's the case, then it's a +1. Otherwise, I'd prefer to have a bit more sublime cases - like checking if we *actually* did modify the netboot tarball.

Revision history for this message
Łukasz Zemczak (sil2100) wrote :

I suspect that it's the case however, as the url is an essential part of the tarball from what I recall?

Revision history for this message
Steve Langasek (vorlon) wrote :

considerations here:
- the information about whether a netboot tarball is being repacked is *very* far removed in the code from where we do the copying of checksums; plumbing this through would be very ick in terms of APIs
- yes, every time in the code that we pass old_dirs to checksum_directory, it's in a context where we would have repacked the netboot tarball
- the use of "old_directories" for checksum_directory is an optimization, to avoid having to recompute checksums over large files (i.e. installer images). The netboot tarballs are on the order of 80MB each, trivial in comparison if there ever were a case where we were needlessly re-checksumming.

Revision history for this message
Łukasz Zemczak (sil2100) wrote :

Okay, I guess this is good enough then!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/lib/cdimage/checksums.py b/lib/cdimage/checksums.py
2index 0e8c939..11e4a5c 100644
3--- a/lib/cdimage/checksums.py
4+++ b/lib/cdimage/checksums.py
5@@ -229,7 +229,7 @@ class ChecksumFileSet:
6 def merge_all(self, old_directories, map_expr=None):
7 images = sorted(
8 name for name in os.listdir(self.directory)
9- if self.want_image(name))
10+ if self.want_image(name) and 'netboot' not in name)
11 for image in images:
12 image_names = [image]
13 if map_expr:

Subscribers

People subscribed via source and target branches

to all changes: