Code review comment for lp:~ajkavanagh/charm-helpers/ppc64le_proposed_fix_plus_refactor

Revision history for this message
Ryan Beisner (1chb1n) wrote :

As you might expect, there are a number of (what seem to be small) merge conflicts with Trunk by now. But I merged this into a local copy of Trunk, took a stab at resolving those conflicts, and found the following issues:

1. Some of the added tests appear to assume that the tests will be executed from a Xenial machine, which will not always be the case. It might be best to base the tests on Xenial (LTS) regardless of the release of the developer's machine which is executing the unit tests.

2. There are a number of "local variable 'env' referenced before assignment" errors in the tests which cover _run_apt_command. This may have been missed with the rebase against that big centos change. This is causing unit and lint check failures.
| charmhelpers/fetch/ubuntu.py:642:57: F821 undefined name 'env'

3. There has been a recent change in the apt retry logic which needs to be reconciled on a rebase. I didn't fully (or carefully) resolve those conflicts in my WIP as I needed to plow on for a separate test case.

http://bazaar.launchpad.net/~charm-helpers/charm-helpers/devel/revision/705

This is causing:
| charmhelpers/fetch/ubuntu.py:645:34: F821 undefined name 'APT_NO_LOCK_RETRY_COUNT'
| charmhelpers/fetch/ubuntu.py:649:29: F821 undefined name 'APT_NO_LOCK_RETRY_DELAY'
| charmhelpers/fetch/ubuntu.py:650:28: F821 undefined name 'APT_NO_LOCK_RETRY_DELAY'

See nosetests results:
 - http://paste.ubuntu.com/24278293/

My temporary trunk merge branch is here (NOT intended to land or to be proposed as my merge conflict resolution approach was quite fast and furious):
 - https://code.launchpad.net/~1chb1n/charm-helpers/lp1611134

I synced it into the nova-compute charm as a WIP TEST here (also NOT for reals):
 - https://github.com/ryan-beisner/charm-nova-compute-wip

And that is what I'm using on arm64 at the moment (!), because I need to consume arm64 ports. I'll chime back in here with how that goes. :-)

review: Needs Fixing

« Back to merge proposal