Code review comment for lp://staging/~evarlast/charms/trusty/apache2/trunk

Hey Jay! I didn't realize when I pointed you at bug 1511474 that you already fixed it way back in November (which made it in with your add-logs-interface merge to apache2 today [0]).

That clears up all the apache2 errors that weren't your fault, but we're not out of the woods yet :) 'make test' with these changes merged into the new apache2 trunk branch yields:

======================================================================
ERROR: tests.test_config_changed.ConfigChangedTest.test_config_changed_ensure_empty_site_dir
----------------------------------------------------------------------
_StringException: Traceback (most recent call last):
  File "/tmp/bundletester-0mKWLv/apache2/.venv/local/lib/python2.7/site-packages/mock/mock.py", line 1305, in patched
    return func(*args, **keywargs)
  File "/tmp/bundletester-0mKWLv/apache2/hooks/tests/test_config_changed.py", line 81, in test_config_changed_ensure_empty_site_dir
    hooks.config_changed()
  File "/tmp/bundletester-0mKWLv/apache2/hooks/hooks.py", line 604, in config_changed
    old_apt_source = open('config.apt-source', 'r').read()
IOError: [Errno 2] No such file or directory: 'config.apt-source'

This happens because install_hook() is the only place config.apt-source is written, and we're not invoking that in test_config_changed.py. You could mock a bunch of stuff out to handle that (like open and whatever subprocess stuff happens in config_changed), but but do you think it would be easier to alter your diff with a try/except during config_changed() in hooks.py?

...
     relationship_data = {}
     config_data = config_get()

+ apt_source = config_data['apt-source']
+ try:
+ old_apt_source = open('config.apt-source', 'r').read()
+ except IOError:
+ # File may not exist if install hook hasn't run (unlikely) or during
+ # ./hooks/tests/test-config-changed.py. Either way, it is safe to
+ # assume if this file DNE, our old apt source == current apt config.
+ old_apt_source = apt_source
...

In the same method, do you need to write the new repo config to the config.apt-source file? Altering your diff like:

+ if old_apt_source != apt_source:
+ subprocess.check_call(['add-apt-repository', '--yes', '-r',
+ old_apt_source])
+ add_source(apt_source, config_data['apt-key-id'])
+ open('config.apt-source', 'w+').write(apt_source)

Otherwise, I don't think your "old_apt_source != apt_source" test will be accurate after changing that config.

One last morsel of awesome, test_config_changed.py needs to close the hookenv.Config with a paren:

- }
+ "apt-source": "",
+ })

I know this MP has been a long time coming, and I *really* appreciate the fixes to apache2/trunk that you didn't cause. I think we're close with this one, but I want to get your input on these last few concerns of mine. Thanks!!

[0] - https://code.launchpad.net/~evarlast/charms/trusty/apache2/add-logs-interface/+merge/278222