Juju Charms Collection
keystone package

Merge lp://qastaging/~pjdc/charms/trusty/keystone/add-admin-relation into lp://qastaging/~openstack-charmers-archive/charms/trusty/keystone/next

Proposed by Paul Collins on 2014-06-25

Status:	Merged
Merged at revision:	70
Proposed branch:	lp://qastaging/~pjdc/charms/trusty/keystone/add-admin-relation
Merge into:	lp://qastaging/~openstack-charmers-archive/charms/trusty/keystone/next
Diff against target:	100 lines (+25/-8) 5 files modified .bzrignore (+1/-0) README.md (+6/-1) hooks/keystone_hooks.py (+16/-6) metadata.yaml (+2/-0) revision (+0/-1)
To merge this branch:	bzr merge lp://qastaging/~pjdc/charms/trusty/keystone/add-admin-relation
Related bugs:	Link a bug report

Reviewer	Date Requested	Status
Liam Young (community)		Approve on 2014-07-07
Nick Moffitt	2014-06-25	Pending
James Page	2014-06-25	Pending
Review via email: mp+224541@code.qastaging.launchpad.net

This proposal supersedes a proposal from 2014-06-23.

Description of the change

Internally, we've created a charm to manage an OpenStack cloud creating users, tenants, and quantum networks. This adds the admin relation providing admin credentials to services such as our internal charm.

This refreshed MP addresses James Page's inline comments on MP#218426:

* admin_relation_changed(): private-address is a unit attribute, so we must use unit_get to fetch it

* remove the inspiration for our pointless do-nothing identity-admin-relation-joined hook, the equally pointless identity-service-relation-joined

Revision history for this message

Nick Moffitt (nick-moffitt) wrote on 2014-05-06: Posted in a previous version of this proposal

2014-05-06 13:47:55 INFO install Traceback (most recent call last):
2014-05-06 13:47:55 INFO install File "/var/lib/juju/agents/unit-keystone-0/charm/hooks/install", line 40, in <module>
2014-05-06 13:47:55 INFO install from keystone_utils import (
2014-05-06 13:47:55 INFO install ImportError: cannot import name stored_passwd
2014-05-06 13:47:55 ERROR juju.worker.uniter uniter.go:486 hook failed: exit status 1

Well now, I'll need to fix that then...

review: Needs Fixing

Revision history for this message

Nick Moffitt (nick-moffitt) wrote on 2014-05-06: Posted in a previous version of this proposal

Okay, looks like stored_passwd got the ALL_CAPS treatment for consistency, and we're good now.

review: Approve

Revision history for this message

James Page (james-page) on 2014-06-23: Posted in a previous version of this proposal

review: Needs Fixing

Revision history for this message

Liam Young (gnuoy) wrote on 2014-06-26:

Can we get a unit test for the new hook and an update to the README to describe the purpose of the new relation pls ?

review: Needs Fixing

Revision history for this message

Nick Moffitt (nick-moffitt) wrote on 2014-06-27:

> Can we get a unit test for the new hook and an update to the README to
> describe the purpose of the new relation pls ?

How exactly does a body unit test a juju relation? It would seem like the very definition of an integration interface. Should we just mock everything out and bake our own optimistic predictions into the test?

Revision history for this message

Liam Young (gnuoy) wrote on 2014-06-27:

I would say "mock everything out and bake our own optimistic predictions into the test" beautifully describes the existing hook tests and would fill the requirement.

Revision history for this message

Nick Moffitt (nick-moffitt) wrote on 2014-06-27:

> I would say "mock everything out and bake our own optimistic predictions into
> the test" beautifully describes the existing hook tests and would fill the
> requirement.

☹

Revision history for this message

Liam Young (gnuoy) wrote on 2014-06-27:

Having said that, the charm now contains some amulet tests. If you wanted to add one of those as well that would give us a more meaningful test.

lp://qastaging/~pjdc/charms/trusty/keystone/add-admin-relation updated on 2014-06-29

70. By Paul Collins on 2014-06-29: README.md: Describe identity-admin and the intended use case.

Revision history for this message

Paul Collins (pjdc) wrote on 2014-07-01:

I'm having trouble identifying the unit test that tests identity-service, which I had hoped to use as a model for the identity-admin test. Can someone point it out to me, please?

I've taken a look at the amulet tests and sketched out the following test plan for the identity-admin relation:

* Deploy keystone, etc, etc.

* Deploy as-yet-unwritten identity-admin demo charm ("guinea-pig").
This charm will create a user and tenant or somesuch.

* Relate keystone to guinea-pig.

* Wait for the environment to settle, and validate the
existence of the user/tenant/whatever.

  * The tests also deploy cinder, if I'm reading them right,
    so the charm could create a volume as well, which would
    demonstrate that the credentials it creates are valid.

Writing the guinea-pig charm isn't really a problem, but putting it in the charm store solely to support a test case seems a bit naff. Could it be stored elsewhere? Failing that, is there some other charm that could be enhanced in some manner to use identity-admin such that it's suitable to use in the amulet tests?

Revision history for this message

Liam Young (gnuoy) wrote on 2014-07-07:

> I'm having trouble identifying the unit test that tests identity-service,
> which I had hoped to use as a model for the identity-admin test. Can someone
> point it out to me, please?
>
> I've taken a look at the amulet tests and sketched out the following test plan
> for the identity-admin relation:
>
> * Deploy keystone, etc, etc.
>
> * Deploy as-yet-unwritten identity-admin demo charm ("guinea-pig").
> This charm will create a user and tenant or somesuch.
>
> * Relate keystone to guinea-pig.
>
> * Wait for the environment to settle, and validate the
> existence of the user/tenant/whatever.
>
> * The tests also deploy cinder, if I'm reading them right,
> so the charm could create a volume as well, which would
> demonstrate that the credentials it creates are valid.
>
> Writing the guinea-pig charm isn't really a problem, but putting it in the
> charm store solely to support a test case seems a bit naff. Could it be
> stored elsewhere? Failing that, is there some other charm that could be
> enhanced in some manner to use identity-admin such that it's suitable to use
> in the amulet tests?

This looks like the correct approach to me and I agree that having a guinea-pig charm in the charmstore is not the correct approach. As I don't have a solution to what the correct approach is at the moment and the amulet test are new I don't think it's appropriate to block on it so I'm going to +1 and merge.

Revision history for this message

Liam Young (gnuoy) on 2014-07-07:

review: Approve