Code review comment for lp:~paelzer/cloud-init/test-apt-source

On Tue, May 24, 2016 at 4:01 PM, Scott Moser <email address hidden> wrote:

> so, some comments inline . that might seem like a lot, and that i'm nit
> picking. Sorry if it seems like that.
>

Having worked on the list of feedback I really found that they are mostly
picks on older code I moved - so I feel good :-)

And for my English sometimes I think I just shouldn't try to race to a new
MP at the end of a day :-)

> > + """
> > + if 'keyid' in ent and 'key' not in ent:
> > + keyserver = "keyserver.ubuntu.com"
> > + if 'keyserver' in ent:
> > + keyserver = ent['keyserver']
> > + try:
> > + ent['key'] = getkeybyid(ent['keyid'], keyserver)
> > + except:
> > + raise Exception('failed to get key from %s' % keyserver)
>
> KeyError ? or LookupError maybe? might seem reasonable.
> catch the subp.ProcessExecutionError specifically or even just let it go
> through would be better than swallowing it with a more generic error.
>
>
Yeah, given a second thought I like not catching it at that level the most.

> >
> > errorlist = []
> > - for ent in srclist:
> > + # convert old list format to new dict based format
>
> converting the array to a dictionary seems like a method itself that would
> be stand alone be easily unit tested.
>

 I first thought to refuse that since so many older tests are testing that
implicitly but ten decided to add it (refusing to go home to the family -
remote this week for better internet access).
Uploading updated MP now ...