On 2017-05-15, Enrico Zini wrote: > On Sun, May 07, 2017 at 09:58:12PM -0700, Vagrant Cascadian wrote: > >> Any chance you could review the urllib implementation that is in git >> soonish? :) > > I've been adventurously busy, I hope it's not too late.
Thanks for the review! :) > In bfc0329060e262bfd889b5e40c32ad9099197dcb I have mixed feelings about > this, as it uses string split on a path while python has os.path.split. > However, I couldn't at a glance follow the code to see what kind of > input it works on, and if it's something like > http://ftp.fr.debian.org/debian/dists/jessie/Release > then the lines are not really paths: > > - prefix_len = 7 + len(env.get("DI_CODENAME")) # > dists/{DI_CODENAME}/ > - relname = file[prefix_len:] > + separator = os.path.join('dists/', > env.get("DI_CODENAME"), '') > + separator, relname = file.split(separator) If I remember correctly, it is trying to split a filename based on dists/CODENAME, so maybe os.path.split would be appropriate here; will look into it. > In b164d54b912a8d702576c191a821ec2f374642cf I agree that except without > an exception is too broad, like in here: > > try: > checksums.verify_file(output, relname) > except: > raise Fail("Checksum invalid: %s", output) > > I noticed that verify_file in simple_cdd/utils.py already throws Fail if > something is wrong. I would get rid of the try/except block altogether, > and just call verify_file. I think with these I was getting an ugly traceback when the file failed to verify, so wanted to actually give the user something more freindly than a traceback... will try your patch again, tweak the file between download and verify, and see if it still spits out the tracebacks. live well, vagrant
signature.asc
Description: PGP signature
