Merge lp://staging/~evarlast/charms/trusty/apache2/trunk into lp://staging/charms/trusty/apache2

This item has failed automated testing! Results available here http://juju-ci.vapour.ws:8080/job/charm-bundle-test-lxc/1486/

This item has failed automated testing! Results available here http://juju-ci.vapour.ws:8080/job/charm-bundle-test-aws/1469/

Hi Jay,

In order to confirm your changes are fully functional, I've attempted to run bundletester (bundletester -e $AWS -l DEBUG -vF) on this.

It appears that there are initial failures related to apt-source, but because of the 'F' I see that there are also older issues related to assert_called_once (see: https://bugs.launchpad.net/charms/+source/apache2/+bug/1511474)

Unfortunately because tests are failing (as noted by the automated testing also) I can't approve this - however, perhaps they're passing for you? If so can you tell me more about your environment so I can try to replicate it - I'm using trusty.

Thanks!
Andrew

Jay,

I've just seen another RQ item from you where you've fixed some of those assert_called_once errors, but still failing on the following:

http://pastebin.ubuntu.com/13898257/

I've put this same pastebin in your other RQ item.

Andrew

Hi Jay,

Just following up from our conversation on IRC yesterday. There are some failing tests here that are in the upstream, but new failing tests are introduced with this merge. Specifically:

======================================================================
ERROR: tests.test_balancer_hook.HooksTest.test_installs_hook
----------------------------------------------------------------------
_StringException: Traceback (most recent call last):
  File "/tmp/bundletester-ylCDXc/apache2/.venv/local/lib/python2.7/site-packages/mock/mock.py", line 1305, in patched
    return func(*args, **keywargs)
  File "/tmp/bundletester-ylCDXc/apache2/hooks/tests/test_balancer_hook.py", line 512, in test_installs_hook
    result = hooks.install_hook()
  File "/tmp/bundletester-ylCDXc/apache2/hooks/hooks.py", line 294, in install_hook
    open('config.apt-source', 'w').write(apt_source)
TypeError: expected a character buffer object

======================================================================
ERROR: tests.test_config_changed.ConfigChangedTest.test_config_changed_ensure_empty_site_dir
----------------------------------------------------------------------
_StringException: Traceback (most recent call last):
  File "/tmp/bundletester-ylCDXc/apache2/.venv/local/lib/python2.7/site-packages/mock/mock.py", line 1305, in patched
    return func(*args, **keywargs)
  File "/tmp/bundletester-ylCDXc/apache2/hooks/tests/test_config_changed.py", line 76, in test_config_changed_ensure_empty_site_dir
    hooks.config_changed()
  File "/tmp/bundletester-ylCDXc/apache2/hooks/hooks.py", line 602, in config_changed
    apt_source = config_data['apt-source']
KeyError: 'apt-source'

Thanks for this. I think I have fixed the required tests.

Please take another look.

These are pretty bad defaults. We're adding the http://ppa.launchpad.net/ondrej/apache2/ubuntu PPA and keys?

I think the defaults for apt-key-id and apt-source should be empty.

Indeed! I intended them to be empty. Thank Haw.

Hi Jay,

There are still tests failing (http://pastebin.ubuntu.com/15017349/) and some of these are the ones Adam pasted further up in this thread. I'm trying to recall if we discussed similar failing tests in another thread. I ran these tests in a charmbox on AWS, perhaps this is unique to my environment?

Maybe someone can help me out with what changes in this MR cause these
tests to fail?

On Thu, Feb 11, 2016 at 6:01 PM, Andrew McLeod
<email address hidden> wrote:
> Review: Needs Fixing
>
> Hi Jay,
>
> There are still tests failing (http://pastebin.ubuntu.com/15017349/) and some of these are the ones Adam pasted further up in this thread. I'm trying to recall if we discussed similar failing tests in another thread. I ran these tests in a charmbox on AWS, perhaps this is unique to my environment?
> --
> https://code.launchpad.net/~evarlast/charms/trusty/apache2/trunk/+merge/278220
> You are the owner of lp:~evarlast/charms/trusty/apache2/trunk.

Hey Jay - it's likely that nothing you did in this MP caused these failures. In fact, I'm quite certain that the 'assert_called_once' failures are happening because 'make test' is pulling in a newer version of mock (v1.3.0) that is more strict about mock'd methods [0].

That said, I agree with Andrew's desire to get tests passing before committing updates to a charm as important as apache2. If you have ideas on how to fix these test issues (even though you didn't introduce them), that would be much appreciated. If not, please at least hit the 'affects me too' on bug [0] to raise the heat.

Meanwhile, I'll try to poke on a test refactor to get around the assert_called_once errors, but we'll still need to dig into the other failures Andrew found in his latest pastebin.

[0] - https://bugs.launchpad.net/charms/+source/apache2/+bug/1511474

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

Great catch @kevin.

I've updated to include suggested fixes.

Thanks for the updates Jay! Tests pass and I verified apt config settings are honored during both install and config-changed.

This has been merged into trusty/apache2. An updated charm should hit the store in about an hour. Thanks again for your patience during this review!