Code review comment for lp:~doanac/ubuntu-ci-services-itself/ppa-cleaner

On 12/16/2013 07:34 AM, Vincent Ladeuil wrote:
> Review: Needs Fixing
>
> 21 +# Read for details: https://help.launchpad.net/API/SigningRequests
> 22 +OAUTH_CONSUMER_KEY = 'TODO'
> 23 +OAUTH_TOKEN = 'TODO'
> 24 +OAUTH_TOKEN_SECRET = 'TODO'
> 25 +
>
> Not sure how this relates to your other MP, I'll consider you've addressed the TODO there for this review.

TODO's are filler. Our juju-deployer configs will override this during
deployment.

> 60 - lockname = bundle.data.get('lockname')
>
> \o/ no more lock ;)

Good catch, revno 25

> 79 + if type(state) != int:
>
> O_o can you add a comment to explain ? As I read it it means we'll be ignoring some requests. I'd rather error out unless you have a strong reason to do so... which will make a perfect comment ;)

good catch, revno 26

> 118 +def lp_collection(url):
>
> You probably want to talk to wgrant about that. If I remember correctly, if the collection is big/huge, you'll run into timeouts and will need to make several requests to acquire parts of the collection.
>
> So better check with wgrant first if you're under a reasonable size for the collection or if you already need to handle chunks.

No issue here. Launchpad returns these back in chunks of items. This
function deal with grabbing those chunks and returning them back
one-by-one so that we don't get overloaded.

> 138 + 'OAuth realm="https://api.launchpad.net/"',
> 139 + 'oauth_version="1.0"',
> 140 + 'oauth_consumer_key="%s"' % settings.OAUTH_CONSUMER_KEY,
> 141 + 'oauth_token="%s"' % settings.OAUTH_TOKEN,
> 142 + 'oauth_signature_method="PLAINTEXT"',
> 143 + 'oauth_signature="%%26%s"' % settings.OAUTH_TOKEN_SECRET,
> 144 + 'oauth_timestamp="%d"' % int(oauth.time.time()),
> 145 + 'oauth_nonce="%s"' % oauth.generate_nonce(),
>
> Great, we already have 3 settings parametrized, I think we also want OAuth realm so we can test against staging or a local launchpad.

revno 27

> 161 + url = 'https://api.launchpad.net/1.0/~%s/+archive/%s/?ws.op='
>
> That one probably needs to be parametrized in the same way.

revno: 28

>
> 249 + logging.exception('unable to request clean ppa, will retry')
>
> Better add the exception encountered if only to start learning about which ones exist.
>
> 266 + logging.exception('error checking if ppa is clean')
>

good catch - revno 29

>
> 354 - # TODO test if this is needed or if there is some type
> 355 - # of looping logic needed
>
> Yeah, see above, not sure you're talking about the same looping logic but that matches what I'm talking about ;)

as noted. this MP actually makes this stuff work properly.

> 364 logging.exception('Could not populate PPAs from launchpad')
>
> Again, mention the exception, the logs are for us and we want precise info ;)

no - good catch. i was thinking logging.exception automatically included
the stack trace.

However - in this case i'm re-raising the exception. So I thought the
caller should log the exception. I was concerned both people would log
the stack trace and it would make it 2x longer to read than needed.

I'll go ahead and log it here, because I don't think my rationale is
strong enough :)

> By just reading the proposal, this seems better than the previous version, so +once the tweaks above are addressed ;)

I'll merge, but please don't hesitate to take a look. I tried to
annotate each of your suggestions with a revno so that it would be easy
for you to see how I responded to your comments.