Ubuntu CI Services

Merge lp://qastaging/~ursinha/ubuntu-ci-services-itself/ppa-add-private into lp://qastaging/ubuntu-ci-services-itself

ppa-add-private
Merge into trunk

Proposed by Ursula Junque on 2014-03-12

Status:

Merged

Approved by:

Ursula Junque on 2014-03-13

Approved revision:

389

Merged at revision:

385

Proposed branch:

lp://qastaging/~ursinha/ubuntu-ci-services-itself/ppa-add-private

Merge into:

lp://qastaging/ubuntu-ci-services-itself

Diff against target:

287 lines (+93/-31)

7 files modified

juju-deployer/configs/unit_config.yaml.tmpl (+2/-0)
ppa-assigner/ppa_assigner/api.py (+6/-2)
ppa-assigner/ppa_assigner/launchpad.py (+10/-3)
ppa-assigner/ppa_assigner/migrations/0001_initial.py (+2/-0)
ppa-assigner/ppa_assigner/models.py (+23/-13)
ppa-assigner/ppa_assigner/settings.py (+1/-0)
ppa-assigner/ppa_assigner/tests.py (+49/-13)

To merge this branch:

bzr merge lp://qastaging/~ursinha/ubuntu-ci-services-itself/ppa-add-private

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration		Approve on 2014-03-13
Francis Ginther			Approve on 2014-03-13
Andy Doan (community)		2014-03-12	Approve on 2014-03-13
Review via email: mp+210704@code.qastaging.launchpad.net

Commit message

This branch adds 'private' to PPA, adds ability to get private and public information from launchpad and adds a variable to define we're using private or public ppas.

Description of the change

This branch adds 'private' to PPA. It was supposed to have signing_key_fingerprint, but now it seems all components are allowed to access launchpad so this is not required.

lp_collection now GETs public and private information, and reserve() only gets ppas according to settings.PRIVATE_ONLY.

Revision history for this message

Ursula Junque (ursinha) wrote on 2014-03-13:

I don't like the check on lp_collection because that's lying. It's not fetching private information only, but *also*.

lp://qastaging/~ursinha/ubuntu-ci-services-itself/ppa-add-private updated on 2014-03-13

384. By Ursula Junque on 2014-03-13: Merging trunk

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-03-13:

PASSED: Continuous integration, rev:384
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/405/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/405/rebuild

review: Approve (continuous-integration)

Revision history for this message

Andy Doan (doanac) wrote on 2014-03-13:

45 +def _lp_get(url):

might be better name _lp_get_authenticated

21 - resp = urllib2.urlopen(url)
22 + if settings.PRIVATE_ONLY:
23 + resp = _lp_get(url)
24 + else:
25 + resp = urllib2.urlopen(url)

Is there really any need to distinguish the 2 code paths? Maybe just always call _lp_get there since you already have the logic in the model to differentiate them?

lp://qastaging/~ursinha/ubuntu-ci-services-itself/ppa-add-private updated on 2014-03-13

385. By Ursula Junque on 2014-03-13: s/_lp_get/_lp_get_authenticated/
386. By Ursula Junque on 2014-03-13: all ppa-assigner/launchpad GETs are now authenticated, bringing also private information

Revision history for this message

Ursula Junque (ursinha) wrote on 2014-03-13:

> 45 +def _lp_get(url):
>
> might be better name _lp_get_authenticated

r385.

>
> 21 - resp = urllib2.urlopen(url)
> 22 + if settings.PRIVATE_ONLY:
> 23 + resp = _lp_get(url)
> 24 + else:
> 25 + resp = urllib2.urlopen(url)
>
> Is there really any need to distinguish the 2 code paths? Maybe just always
> call _lp_get there since you already have the logic in the model to
> differentiate them?

I think I was only being overcautious about always getting private information (as other methods also use lp_collection), but at this point I think it shouldn't matter really. As I said before I don't even like it, hehe. Fixed on r386.

Do you think these tests are enough? I deployment similar code yesterday and it seemed to me reserve() wasn't respecting the settings.PRIVATE_ONLY variable, and I couldn't find any logs (gunicorn issue, I think). Is there anything else I need to do besides setting that on unit_config? Tests are passing now but maybe I'm not testing this as I should.

Revision history for this message

Ursula Junque (ursinha) wrote on 2014-03-13:

> Do you think these tests are enough? I deployment similar code yesterday and
> it seemed to me reserve() wasn't respecting the settings.PRIVATE_ONLY
> variable, and I couldn't find any logs (gunicorn issue, I think). Is there
> anything else I need to do besides setting that on unit_config? Tests are
> passing now but maybe I'm not testing this as I should.

s/deployment/deployed/

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-03-13:

PASSED: Continuous integration, rev:386
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/406/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/406/rebuild

review: Approve (continuous-integration)

Revision history for this message

Andy Doan (doanac) wrote on 2014-03-13:

is it possible you failed to restart django after changing your unit-config and/or settings.py?

139 - qs = PPA.objects.select_for_update().filter(state=PPA.AVAILABLE)
140 + qs = PPA.objects.select_for_update().filter(
141 + state=PPA.AVAILABLE, private=settings.PRIVATE_ONLY)

That looks pretty cut-and-dry to me. I don't think that could do the wrong thing, and:

190 + def testReservePrivateOnly(self):

seems to verify that block

review: Approve

lp://qastaging/~ursinha/ubuntu-ci-services-itself/ppa-add-private updated on 2014-03-13

387. By Ursula Junque on 2014-03-13: Forgot this: engine health page should show total of PPAs available for privacy set
388. By Ursula Junque on 2014-03-13: Making tests check if engine health will display only private/public ppas