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

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.

60 - lockname = bundle.data.get('lockname')

\o/ no more lock ;)

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 ;)

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.

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.

161 + url = 'https://api.launchpad.net/1.0/~%s/+archive/%s/?ws.op='

That one probably needs to be parametrized in the same way.

223 + # TODO check for oauth before launching

Sounds like a good test to add ;)

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')

idem

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 ;)

364 logging.exception('Could not populate PPAs from launchpad')

Again, mention the exception, the logs are for us and we want precise info ;)

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