Quick one to get the juices flowing.

I recently started a new job, and I have been starting to dip my toes into reviewing pull requests the last two weeks. I wanted to take a minute to share my thoughts on a mistake I see even experienced engineers make fairly regularly. It seems silly, but it’s a real footgun, and I see it all the time.

Today, a colleague posted a PR for review that included the following line:

if settings.ENVIRONMENT == DEVELOPMENT_ENVIRONMENT:
    <...>

This is a seemingly innocuous line. It is also an operational nightmare. In certain limited situations, it is alright for the program to check its current execution environment directly, but generally you have better options and you should avoid doing so.

The problem here is two-fold, as outlined by @justjake in a great tweet I can no longer find.

Our first problem is that we are coupling an execution decision directly to the environment, when there’s secretly an intermediary logical leap that is being left off the page. Let’s say the above code was part of the larger statement below:

if settings.ENVIRONMENT == DEVELOPMENT_ENVIRONMENT:
    logger.trace.redirect = ‘/dev/null’

Here we see our program checking whether we are in dev, and redirecting trace logs to /dev/null. One might want to do this in order to tame a crazy log stream, or because they don’t have a tracer running locally that can process these logs into something useful. The logical leap I mentioned earlier is exactly that sentence. Why are we redirecting logs to /dev/null? In fact, part of the reason I chose such an odd example is exactly because it is unexplainable without that bit of context. it’s a real head-scratcher of a decision if you see it just like that on its own! How would you do this correctly? Like so:

if config.logging.mute_tracer_noise == true:
    logger.trace.redirect = ‘/dev/null’

Above, we at least get some more insight into why the decision was made. We can read into the flag name a little and understand that the original implementer wanted to quiet the trace logs, not that they didn’t have an appropriate transport available.

It also means that, if our tracer ever gets too noisy (or costly!) in another environment, we can easily go into our environment config files and change the value. Which takes us to the second code smell coming out of the original approach!

Let’s go back to our original formulation, but say that we’ve added a new environment where we want to mute the tracer. Let’s call it UAT. Updating the original code to satisfy this requirement would look like this:

if settings.ENVIRONMENT == DEVELOPMENT_ENVIRONMENT or settings.ENVIRONMENT == UAT_ENVIRONMENT:
    logger.trace.redirect = '/dev/null'

Ugly! More importantly though, it's operationally intensive. If we were adding new environments weekly, eventually we’d be managing a function just to generate this boolean!

This is actually the more insidious aspect of this code smell. There is now implicit coupling of every configuration that depends on the settings.ENVIRONMENT variable. If we ever want to temporarily enable the tracer in our development environment, the correct solution is now not necessarily clear! The correct solution would be to modify our conditional to drop the DEVELOPMENT_ENVIRONMENT, or delete the conditional completely. How many among us could seriously say that we wouldn’t lazily take an alternate approach at least once, by using the debugger to set settings.ENVIRONMENT to “TEST”? Well, now every downstream configuration will also behave like production, including the one that controls whether you drop the database every time the application terminates.

Wait, your dev machines can’t connect to your test environment directly, right?

There’s a few more examples I could cite, but I wanted this to be fast. You should be using your configuration files to do configuration, and only the truly necessary, bootstrap-related decision making in your app should be coupled to the environment directly.

I’ll leave you with one final note: what happens to your environment-coupled configs when the variable you’re checking comes up null? Are you sure all your conditionals are safe? Even if you’re using a configuration file like I recommend, make sure that you have a good strategy for enforcing the presence of default values, and something to deal with confusing and deeply-nested overrides.

Cheers.