search

Nihilate / Roboragi

Roboragi is a Reddit bot which helps link anime, manga, and other Japanese media.
GNU Affero General Public License v3.0
167 stars 30 forks source link

Improve the config file #21

Open Nihilate opened 6 years ago

Nihilate commented 6 years ago

At the moment there's no sample configuration file, and the one that is being used in "prod" is just a simple Python file that gets read from. For the sake of future proofing with other environments in mind (as well as sharing a sample file with other users), the file should be changed to reflect whatever the industry standard is (or a least use a standard) - YAML or JSON or something.

Nihilate commented 6 years ago

As per my notes in #46, it would be good to be able to allow PRs from forks into the CI/CD pipeline, but in order to do that I need a way to keep the secrets secret.

Nihilate commented 6 years ago

In order to get #48 a bit nicer, I'm going to grab this as well.

reillysiemens commented 6 years ago

On the topic, can you provide a sanitized config? Perhaps in a gist? I've never seen the real module, only the shape of it via usage. Would it be possible to prefer environment variables rather than hard-coded values? Or is the config sufficiently in need of structured sections to make that a non-starter?

reillysiemens commented 6 years ago

FWIW, as far as structured, serializable formats go I've seen the Python community trending toward TOML over YAML or JSON lately.

See rationale like that of PEP518 for why TOML might be a preferable option.

The Pipfile project is also using TOML as its human-readable format.

Nihilate commented 6 years ago

There's a sanitised version of the current config here. The majority of it (usernames, passwords, connection strings) can and will be moved into environment variables unless there's a good argument against it - it makes deployment, containerisation and keeping things secret way easier. I was thinking it would be nice to have the option of using a file when developing locally, so I'll probably add support for that as well (get environment variable by default, if not present look in the file).

The signature stuff can go somewhere closer to comment formatting/generation, although it would be good to have at least part of that as config (I like to put a custom link in every so often).

There's also the matter of the subreddit list and the stuff in here, which is a bit more fluid and too large to reasonably handle as an environment variable. Managing it via the database doesn't sound like a terrible idea to me, but I'm open to ideas.

dashwav commented 6 years ago

My personal vote on this matter goes to YAML. It is a more well-established config format that I personally think is actually more readable than TOML. Most of my actual issues with TOML is better summarized in this reddit comment and in this comment on a comparison GIST.

That being said, the choice isn't necessarily set in stone as they are very compatible with each other and as long as we do everything right, the only thing we need to do to switch between them is change the config file and the loader library we are using in our code

Nihilate commented 6 years ago

They look reasonably similar and I'm not fussed one way or another, so YAML sounds good to me.

reillysiemens commented 6 years ago

FWIW, my personal vote on the matter still goes to TOML, mainly for the reasons I outlined above, but also because of the positive experiences I've had with it when working in Rust. I think that if your config file dosen't get too complicated or deeply nested then TOML is a good choice.

That said, @dashwav has already done the legwork of adding YAML support in #62 and having something that works goes a long way for me. :laughing: Basically, I'm not fussed about it either and I'm grateful to @dashwav for having picked something and made it happen (I think either choice is better than a .py config file).

Now that there's a YAML config, is there anything else keeping this issue open? Are there improvements we still want to make here? Putting the database/secret configs into the file as well? Or do we want to put most of that information into environment variables?

Nihilate commented 6 years ago

Are there improvements we still want to make here? Putting the database/secret configs into the file as well? Or do we want to put most of that information into environment variables?

Sorry, I've been meaning to get around to this - everything could do with moving from the old .py config before making this as completed. Sticking secrets into a config file makes things a little trickier for CI, but it would also be nice to manage everything the same way - I'm not sure splitting some things into environment variables makes that much sense any more.

dashwav commented 6 years ago

What I have done in the past is use an argument flag (-t or --test) and if that flag is passed in, look for environment variables for db creds and other such 'secret' credentials, otherwise load from config file. That way you can still do CI/CD easily. You can kind of see the code here.

reillysiemens commented 6 years ago

@dashwav's suggestion seems reasonable to me, but I would go so far as to say that even in production secrets should be pulled from an environment variable rather than a file if possible.