jalmeroth / homie-python

A Python-implementation of the homie v2 convention.
https://github.com/marvinroger/homie
54 stars 15 forks source link

Configuration #6

Closed jpmens closed 8 years ago

jpmens commented 8 years ago

I've resorted to this in order to adjust configuration parameters for testing:

config = {
    "HOST": "localhost",
    "PORT": 1888,
    "KEEPALIVE": 10,
    "DEVICE_ID": "xxxxxxxx",
    "DEVICE_NAME": "xxxxxxxx",
    "TOPIC": "devices"
}

Homie = homie.Homie(**config)

I wonder if it would be sensible to be able to pass a filename to homie.Homie() for it to read the configuration from. Alternatively, maybe support configuration via environment variables?

jpmens commented 8 years ago

Additionally, it would be good to have support for TLS (needs a filename to a PEM-encoded CA certificate) as well as client authentication (username, password)

jalmeroth commented 8 years ago

(Note: I opened new issues for Authentication #7 and TLS #8.)

@jpmens which kind of configuration file format do you think would suit best here? JSON, YAML, …?

jpmens commented 8 years ago

JSON would have the advantage that you could reuse Marvin's Homie-esp8266 configuration file format, and that JSON is built-in to Python.

YAML looks neater and is easier to edit, but requires additional "batteries". If we want "editable" I think something parseable by ConfigParser would do the trick (e.g. "Ini").

Assuming that homie-python will be used by people who also use homie-esp8266, I'd go for Marvin's format.

jalmeroth commented 8 years ago

@jpmens thanks for your thoughts! 👍

You can now pass on a configuration file (in JSON) to homie-python.

jpmens commented 8 years ago

If I ever get around to doing this (although it's trivial), would you accept a PR which adds the possibility of setting device ID/name from an environment variable (HOMIE_DEVICEID, say)? This would enable programs to use the same configuration file but override certain bits from the environment.

jpmens commented 8 years ago

The configuration from file works, @jalmeroth, which is good.

What I consider an issue though, is that if the config cannot be opened (e.g. because the specified file doesn't exist), you fall back to default values, in particular host test.mosquitto.org where my "device" would publish retained values to. This could be a privacy issue.

I think it would be better to fail completely if the JSON configuration cannot be opened.

jalmeroth commented 8 years ago

@jpmens sure, I would accept your PR… but how about this:

    self.deviceId = self.config.get("DEVICE_ID", (
        os.environ["HOMIE_DEVICE_ID"]
        if "HOMIE_DEVICE_ID" in os.environ
        else "xxxxxxxx"))

Does this fulfill your needs?

jpmens commented 8 years ago

That's along the lines of what I was thinking, yes. (Except that your code is much cleaner than mine would be. :-)

But isn't this easier?

self.deviceId = self.config.get("DEVICE_ID", os.getenv("HOMIE_DEVICE_ID", "xxxxxx"))
jalmeroth commented 8 years ago

Added environment variables in 539dc15f610e8b02125618099fcae973d2915274

jpmens commented 8 years ago

Just a thought: we could override each of the config values with variables in loadConfig:

config = json.load(fp)
...
for key in config:
    config[key] = os.getenv("HOMIE_" + key, config[key])
jalmeroth commented 8 years ago

Hm, I personally don't use environment that much. What would be the use-case for this approach?

jpmens commented 8 years ago

Being able to test by overriding from environment without having to provide a distinct configuration file.

jalmeroth commented 8 years ago

I introduced an overwrite-function and defined the list pref_keys allowed to be overwritten. Does this fit?

jpmens commented 8 years ago

This fits. Thank you.