Code review comment for lp:~cprov/uci-engine/charm-tests

Revision history for this message
Vincent Ladeuil (vila) wrote :

There are many issues with the MP, the main ones are:
- it breaks --list

- it uses imp.load_module() whose doc says: "This function does more than importing the module: if the module was already imported, it is equivalent to a reload()!" (The question mark is theirs). See https://docs.python.org/2/library/functions.html?highlight=reload#reload and the endless list of issues associated with that function.

There is no way we can trust reload(), it's meant to be used interactively with a full understanding of the fallouts.

The root issue that you're pointing out here is that we're using hooks.py to mean different things, python doesn't allow that. So that's what need to be changed.

I had a look at it and the second similar issue is that we *also* do 'import charmhelpers' with various definitions across charms and also try to convince python that they are all the same. This one is slightly simpler to fix: as a project, we should use the *same* version in all charms (we do use the same revno but build a different python package via different charm-helpers.yaml files).

I think it's time to stop using bigger hammers that do more harm than good, step back to the original decision to rely on 'import hooks' and fix that.

review: Needs Fixing

« Back to merge proposal