Code review comment for lp:~chris-gondolin/charms/trusty/storage/trunk

Revision history for this message
Cory Johns (johnsca) wrote :

Chris,

Thanks for adding the tests for this functionality. The new tests look good, but unfortunately I had some issues running them.

First, I'd just like to point out that Amulet has been updated to fix the issues you are working around and mention in your tests, specifically the issues with the series and local charm not being used properly, and adding relations after deployment not working correctly. So, you can change your d.add('postgresql', 'cs:trusty/postgresql-5') and d.add('storage', '/home/chris/work/charms/trusty/storage') to just d.add('postgresql') and d.add('storage'). Regarding the post-deployment relations, the only thing that you should need to change is to add d.sentry.wait() after adding the relation, to ensure that the system has time to process the change.

That said, with those changes I am still getting failures (http://pastebin.ubuntu.com/8838368/), as well as hook failures (http://pastebin.ubuntu.com/8838352/). This happened on both lxc and aws, on newly bootstrapped environments.

Additionally, `make lint` had some complaints about the new code, just some small quibbles about line length and spacing: http://pastebin.ubuntu.com/8837062/

Finally, some of the existing tests in `make test` are having issues with the new code. It mostly seems like they need some additional mocks (http://pastebin.ubuntu.com/8837040/) or new data values (http://pastebin.ubuntu.com/8837008/), but one in particular seems like it might be an issue in the new code: http://pastebin.ubuntu.com/8837016/

review: Needs Fixing

« Back to merge proposal