Juju Charms Collection
nova-compute package

Merge lp://qastaging/~brad-marshall/charms/precise/nova-compute/instance-path-config into lp://qastaging/~openstack-charmers-archive/charms/precise/nova-compute/trunk

  1. Precise Pangolin (12.04)
  2. instance-path-config
  3. Merge into trunk
Proposed by Marco Ceppi
Status: Merged
Merged at revision: 58
Proposed branch: lp://qastaging/~brad-marshall/charms/precise/nova-compute/instance-path-config
Merge into: lp://qastaging/~openstack-charmers-archive/charms/precise/nova-compute/trunk
Diff against target: 106 lines (+31/-1)
7 files modified
config.yaml (+3/-0)
hooks/nova_compute_context.py (+3/-0)
hooks/nova_compute_hooks.py (+6/-1)
hooks/nova_compute_utils.py (+5/-0)
templates/folsom/nova.conf (+5/-0)
templates/grizzly/nova.conf (+5/-0)
templates/havana/nova.conf (+4/-0)
To merge this branch: bzr merge lp://qastaging/~brad-marshall/charms/precise/nova-compute/instance-path-config
Reviewer Review Type Date Requested Status
Jacek Nykis (community) Needs Resubmitting
James Page Needs Fixing
Review via email: mp+207167@code.qastaging.launchpad.net

This proposal supersedes a proposal from 2014-02-19.

Description of the change

Added an instances-path variable to the config that allows setting of instances_path in nova.conf. It will also ensure the directory is owned by user nova.

To post a comment you must log in.
Revision history for this message
James Page (james-page) wrote :

Hi Brad

Thanks for this merge-proposal - looks useful.

Despite what charm proof tells you, I think not providing a default value is the right thing todo here - it avoids having todo things like:

  if config('instances-path') != '':
     ...

as config() will return None for an unset value - resulting in cleaner code IMHO:

  if config('instances-path') is None:
     ...

Please can you update inline with this.

review: Needs Fixing
Revision history for this message
Brad Marshall (brad-marshall) wrote :

Done, I've updated the code to reflect your comments - please let me know if there's any other issues.

Revision history for this message
Jacek Nykis (jacekn) wrote :

> Hi Brad
>
> Thanks for this merge-proposal - looks useful.
>
> Despite what charm proof tells you, I think not providing a default value is
> the right thing todo here - it avoids having todo things like:
>
> if config('instances-path') != '':
> ...
>
> as config() will return None for an unset value - resulting in cleaner code
> IMHO:
>
> if config('instances-path') is None:
> ...
>
> Please can you update inline with this.

Hi James,

This has been fixed. Would you mind re-reviewing the MP?

review: Needs Resubmitting
Revision history for this message
James Page (james-page) wrote :

Sorry - missed the update in the torrent of email.

Revision history for this message
James Page (james-page) wrote :

Sorry - I missed this first time round; this change makes the current unit tests fail - the context tests will need some more patching for the use of config:

  make test

It would also be nice to see some tests for this change as well to make sure config is propagated correctly.

(hopefully proposed merges will be tested automatically for unit test and lint failures in future so you don't have to rely on me running the tests).

review: Needs Fixing
lp://qastaging/~brad-marshall/charms/precise/nova-compute/instance-path-config updated
59. By Jacek Nykis

Merged upstream changes

60. By Jacek Nykis

Fixed bug in config-changed

Revision history for this message
Jacek Nykis (jacekn) wrote :

I fixed small logic bug in the config-changed hook, all tests pass now.

review: Needs Resubmitting

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches