Empty files cause issues while loading

[jmckitrick]

When using cprop as part of the luminus framework, there is a config.edn for each environment: dev, test, and prod. The files generated by luminus are mostly empty, and the developer can add settings if needed.

When pushing to prod on Heroku, however, these empty files cause a problem. We get an error saying a non-empty config file was not found. The file is present, but has an empty map {}. The workaround is to put a redundant or meaningless value in the map, but it seems the system should handle an empty map.

[jprudent]

@tolitius Hi :) Is that a bug or a feature ? I encountered a similar use case while using cprop (and same workaround).
I could propose a fix but I need your opinion first.

The fix would be to change
https://github.com/tolitius/cprop/blob/master/src/cprop/core.cljc#L20
with something equivalent to

(if-let [config (cond (map? config) config (nil? config) {} :else nil)]) 

(This is an idea, not the final shape of the code)
Thanks.

[jmckitrick]

I think that would work fine. I'm not sure why the code doesn't read {} as a map already, though.

[jprudent]

I think that would work fine. I'm not sure why the code doesn't read {} as a map already, though.

Because of not-empty : (is (= nil (not-empty {})))

[jmckitrick]

Ah, ok. I hadn't dug into the code, but that makes sense.

[tolitius]

Is that a bug or a feature?

debatable :)
it's a balance between:

• this library is unnecessary in case there is no config to load
• a config is truly missing, but it is not clear and this error is explicitly calling it out

the second one occurs more often, hence the behavior

[jmckitrick]

So is a change warranted, or do I just need to be aware of this behavior and work around it?

[Quantisan]

Just ran into this. The design choice of assuming that load-config is the one and only entry point seems limiting to me. @tolitius, I like what you did here with providing structure to env vars. So sometimes I might not want to load a config file but still want to use from-env to give me a nested map.

In terms of 12-factor, it's foreseeable that on dev you'd use load-config; but on prod, you'd use from-env. Having 1) load-config as the only entry-point to this library, and 2) making it crash the app if there's no config.edn, more so if it's non-empty, 3) assume that from-env only gets passed as an arg to load-config, seems like limiting design choice.

For sake of composability, why not raise load-config, from-env, from-system-props as top-level fns to this library? then everyone can have their cake and eat it too. All that we need is t/merge-maps also on a top-level and we can compose the different sources of properties as we see fit.

[Quantisan]

FYI I'm doing this to enable load-config with a config.edn on dev, and fallback to from-env without a config.edn on prod. I don't want to use t/merge-maps in fear of reaching into implementation too much.

(try
    (load-config :merge [(from-system-props) (from-env)])

    (catch RuntimeException ex
      ;; https://github.com/tolitius/cprop/issues/53#issuecomment-912153331
      (if (re-seq #"non empty configuration file" (.getMessage ex))
        (from-env)
        (throw ex))))