juju-ci-tools

Merge lp://staging/~veebers/juju-ci-tools/initial-log-forwarding-tests into lp://staging/juju-ci-tools

initial-log-forwarding-tests
Merge into trunk

Proposed by Christopher Lee on 2016-07-14

Status:	Approved
Approved by:	Martin Packman on 2016-08-04
Approved revision:	1510
Proposed branch:	lp://staging/~veebers/juju-ci-tools/initial-log-forwarding-tests
Merge into:	lp://staging/juju-ci-tools
Diff against target:	707 lines (+661/-0) 7 files modified assess_log_forward.py (+282/-0) certificates.py (+139/-0) jujupy.py (+7/-0) log_check.py (+55/-0) tests/test_assess_log_forward.py (+96/-0) tests/test_jujupy.py (+29/-0) tests/test_log_check.py (+53/-0)
To merge this branch:	bzr merge lp://staging/~veebers/juju-ci-tools/initial-log-forwarding-tests
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Martin Packman (community)		2016-07-14	Approve on 2016-08-04
Review via email: mp+300039@code.staging.launchpad.net

Commit message

Add initial test for log forwarding feature

Description of the change

Adds initial test for the log-forwarding feature.

Handles creation of certificates tied to the rsylog sync as required by the feature

Bootstraps 2 environments, one being the rsyslog sink for logs the other being the emitter of logs.

Has a basic check that exercises the feature, there is further tests to be added but this MP is getting large as it is.

I added a new file 'certificates.py'. I was tempted to create a python package but this isn't a small step and wanted to converse with the team if it was the direction we wanted to go.

I.e. I would propose something like this initially:
  juju_ci_tools/
    __init__.py
    certificates.py

lp://staging/~veebers/juju-ci-tools/initial-log-forwarding-tests updated on 2016-07-14

1496. By Christopher Lee on 2016-07-14: Fix assertion messages.
1497. By Christopher Lee on 2016-07-14: Remove yet to be used methods.

Revision history for this message

Martin Packman (gz) wrote on 2016-07-14:

Have a bunch of random comments inline. Overall looks like a good approach, my main points of feedback:

* Assess scripts this complex can benefit from a bit more structure. A bunch of cooperating functions gets hard to follow around the 200 line mark. An encapsulating class for each environment seems reasonable.

* A bunch of the setup work is done by manual ssh steps on charms. Ideally, most of this complexity is encapsulated in the charm instead.

* I'd generally have more unit tests, but can see how a bunch of the current code is not amenable to it.

Revision history for this message

Martin Packman (gz) wrote on 2016-07-14:

This time with the comments...

Revision history for this message

Christopher Lee (veebers) wrote on 2016-07-19:

Responded to comments ans made some changes. Still have a couple of changes to make.

lp://staging/~veebers/juju-ci-tools/initial-log-forwarding-tests updated on 2016-07-19

1498. By Christopher Lee on 2016-07-19: Grammar fix and import fix.
1499. By Christopher Lee on 2016-07-19: Better naming for boostrap managers.
1500. By Christopher Lee on 2016-07-19: Change certificate details to not be a getter function.

Revision history for this message

Martin Packman (gz) wrote on 2016-07-21:

Replied to some comments.

lp://staging/~veebers/juju-ci-tools/initial-log-forwarding-tests updated on 2016-07-28

1501. By Christopher Lee on 2016-07-25: Merge trunk
1502. By Christopher Lee on 2016-07-26: Rename and reduce assess function
1503. By Christopher Lee on 2016-07-28: Using python script run on remote machine for logs check.
1504. By Christopher Lee on 2016-07-28: Add failling test for get_controller_uuid
1505. By Christopher Lee on 2016-07-28: Make test pass for added get_controller_uuid
1506. By Christopher Lee on 2016-07-28: Modify test to use get_controller_uuid
1507. By Christopher Lee on 2016-07-28: Cater for ipv6 addresses + port. Incl. tests.
1508. By Christopher Lee on 2016-07-28: Fix remote python script.