OpenStack Compute (nova)

Merge lp://qastaging/~xtoddx/nova/disable_creds into lp://qastaging/~hudson-openstack/nova/trunk

disable_creds
Merge into trunk

Proposed by Todd Willey on 2011-03-23

Status:	Work in progress
Proposed branch:	lp://qastaging/~xtoddx/nova/disable_creds
Merge into:	lp://qastaging/~hudson-openstack/nova/trunk
Diff against target:	56 lines (+27/-0) 2 files modified nova/adminclient.py (+21/-0) nova/api/ec2/admin.py (+6/-0)
To merge this branch:	bzr merge lp://qastaging/~xtoddx/nova/disable_creds
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Christopher MacGown (community)			Needs Fixing on 2011-05-03
termie (community)		2011-03-23	Approve on 2011-03-30
Thierry Carrez (community)	ffe		Needs Information on 2011-03-25
John Dewey (community)			Needs Fixing on 2011-03-23
Review via email: mp+54453@code.qastaging.launchpad.net

Description of the change

In the case of compromised vpn credentials, we need to be able to terminate the affected vpn and revoke the credentials. This adds that functionality to the ec2 admin api.

Revision history for this message

John Dewey (retr0h) wrote on 2011-03-23:

I do not see tests for adminclient.py, but maybe now is a good time to add them?

I have a hard time approving a feature that lacks tests, even if those before us didn't add them.

review: Needs Fixing

Revision history for this message

Thierry Carrez (ttx) wrote on 2011-03-25:

We just hit feature freeze, if you want this into Cactus rather than wait for Diablo, could you provide a quick benefit vs. risk of regression rationale, then request a specific FFe review from me ?

Revision history for this message

Thierry Carrez (ttx) on 2011-03-25:

review: Needs Information (ffe)

Revision history for this message

termie (termie) wrote on 2011-03-25:

diffline 33: please put StatusResponse on the next line, indented with the other args.

should we be internationalizing the response in the disable project creds call?

also looks like you'll need a newline before the StatusResponse class def.

Other than that, looking good. Other folks's comments still applicable, though I think tests will need to be in smoketests if you write them.

review: Needs Fixing

lp://qastaging/~xtoddx/nova/disable_creds updated on 2011-03-29

847. By Todd Willey on 2011-03-29: merge trunk
848. By Todd Willey on 2011-03-29: Fix style issues and add i18n.

Revision history for this message

Todd Willey (xtoddx) wrote on 2011-03-29:

Merged trunk and fixed style.

Decided against i18n as clients may be different language than api server.

lp://qastaging/~xtoddx/nova/disable_creds updated on 2011-03-29

849. By Todd Willey on 2011-03-29: Revert i18n on server-side.

Revision history for this message

termie (termie) wrote on 2011-03-30:

style-wise looks good, smoketests would be great

review: Approve

Revision history for this message

Matt Dietz (cerberus) wrote on 2011-04-19:

Looks good to me, though I agree that tests would be lovely.

Revision history for this message

Christopher MacGown (0x44) wrote on 2011-05-03:

Per the new testing policy decided at the Diablo summit, this needs tests before it can be merged.

review: Needs Fixing

Revision history for this message

Christopher MacGown (0x44) wrote on 2011-05-03:

> Per the new testing policy decided at the Diablo summit, this needs tests
> before it can be merged.

Otherwise, it looks good.

Unmerged revisions

849. By Todd Willey on 2011-03-29: Revert i18n on server-side.
848. By Todd Willey on 2011-03-29: Fix style issues and add i18n.
847. By Todd Willey on 2011-03-29: merge trunk
846. By Todd Willey on 2011-03-23: Admin api methods for disabling credentials & terminating vpns.