I have long believed that it is ok to make any given mistake once. But to make it again is, I believe, unforgivable.
This should, I believe, apply to all things that we do as software developers, and to that end, I feel that code changes to fix issues should be backed up in some way by testing that prevents recurrence of problems.
In my day job, I work on the OpenStack Trove project, and when I review changes proposed by others, I tend to apply this yardstick. When someone fixes a line of code to correct some issue, it is common to expect that there will be a test to verify this operation in the future.
Recently, I reviewed and approved a change that pretty immediately resulted in a regression. Here’s the diff of the code in question:
if not manager:
- msg = ("Manager class not registered for datastore manager %s" %
+ msg = (_LE("Manager class not registered for datastore manager %s") %
CONF.datastore_manager)
raise RuntimeError(msg)
Not being a compiled language, and since no tests exist for the case where manager is None, this code was never exercised, and _LE was not defined. Sure enough a couple of days later, the regression was reported.
This got me wondering how the problem could be avoided. Surely python must have some tools to catch this kind of thing. How did this escape the development process (obvious explanation of sloppy code review aside).
It turns out that there is a mechanism to catch these kinds of things, pylint. And it turns out that we don’t use pylint very much in OpenStack.
Well, a short while later I was able to run my little pylint based wrapper on Trove and fix some egregious bugs.
pylint doesn’t natively give you a way to provide a specific set of issues that must be ignored (something which bandit does). So I modeled this wrapper on the way bandit does things and allowed for a set of ignored exceptions.
I’ll make this a job in the Trove gate soon and that will help stamp out these kinds of issues more quickly.
Hype Cycles 