Juju Charms Collection
contrail-configuration package

Merge lp://staging/~dmitriis/charms/trusty/contrail-configuration/trunk into lp://staging/~sdn-charmers/charms/trusty/contrail-configuration/trunk

  1. Trusty Tahr (14.04)
  2. trunk
  3. Merge into trunk
Proposed by Dmitrii Shcherbakov
Status: Merged
Merged at revision: 68
Proposed branch: lp://staging/~dmitriis/charms/trusty/contrail-configuration/trunk
Merge into: lp://staging/~sdn-charmers/charms/trusty/contrail-configuration/trunk
Diff against target: 655 lines (+202/-70)
4 files modified
config.yaml (+10/-0)
hooks/contrail_configuration_hooks.py (+171/-67)
hooks/contrail_configuration_utils.py (+11/-2)
templates/contrail-api.conf (+10/-1)
To merge this branch: bzr merge lp://staging/~dmitriis/charms/trusty/contrail-configuration/trunk
Reviewer Review Type Date Requested Status
Robert Ayres (community) Approve
Ante Karamatić Pending
Review via email: mp+320826@code.staging.launchpad.net

This proposal supersedes a proposal from 2017-03-19.

Description of the change

rbac support (rebased)

To post a comment you must log in.
Revision history for this message
Ante Karamatić (ivoks) wrote : Posted in a previous version of this proposal

See my comment

review: Needs Fixing
Revision history for this message
Ante Karamatić (ivoks) wrote : Posted in a previous version of this proposal

One more comment

review: Needs Fixing
Revision history for this message
Ante Karamatić (ivoks) wrote : Posted in a previous version of this proposal

I haven't investigated into detail, but with your patches contrail-analytics never populates 'api_server' in /etc/contrail/contrail-analytics-api.conf, which in turn means that contrail-analytics is not functional.

Revision history for this message
Dmitrii Shcherbakov (dmitriis) wrote : Posted in a previous version of this proposal

Ante,

Not sure about api_server - no modifications for that in my MP.

https://code.launchpad.net/~dmitriis/charms/trusty/contrail-analytics/trunk/+merge/320154

Have to investigate why.

Have you tried it without my patch or just with the patch?

If not, we can try it without a patch first to figure out if I introduced a regression or not.

Revision history for this message
Bernhard Koessler (bkoessler) wrote : Posted in a previous version of this proposal

I would recommend not using multi_tenancy anymore going forward.

aaa_mode can be set to:

no-auth—No authentication is performed and full access is granted to all.
cloud-admin—Authentication is performed and only the admin role has access.
rbac—Authentication is performed and access is granted based on role.

cloud-admin would be the same behaviour as multi-tenancy=true

Revision history for this message
Ante Karamatić (ivoks) wrote : Posted in a previous version of this proposal

Right, but charms need to support older versions also.

Revision history for this message
Dmitrii Shcherbakov (dmitriis) wrote :

Just noticed a piece of dead code for an action that we wanted to implement originally - uploaded an updated branch.

Revision history for this message
Robert Ayres (robert-ayres) wrote :

For this to get merged, please remove all the unnecessary formatting changes.

This diff should only contain the *actual* code changes.

Revision history for this message
Dmitrii Shcherbakov (dmitriis) wrote :

Robert,

They were separated into two revisions intentionally and it is possible to view the individual diffs.

https://bazaar.launchpad.net/~dmitriis/charms/trusty/contrail-configuration/trunk/revision/66

https://bazaar.launchpad.net/~dmitriis/charms/trusty/contrail-configuration/trunk/revision/67

Lint checks are a normal part of the charm dev process so I think it is worthwhile that they pass:

https://docs.openstack.org/developer/charm-guide/testing.html#lint
https://jujucharms.com/docs/2.1/authors-charm-best-practice#charm-coding-guidelines

Revision history for this message
Robert Ayres (robert-ayres) wrote :

I appreciate the lint comment, but it would be better if changes to pass lint tests were in a separate patch.

Revision history for this message
Robert Ayres (robert-ayres) wrote :

To save effort, we can just look at merging r67 here.

Revision history for this message
Robert Ayres (robert-ayres) :
review: Approve

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