Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improvement about config.py #786

Closed
magsout opened this issue Oct 22, 2015 · 10 comments
Closed

improvement about config.py #786

magsout opened this issue Oct 22, 2015 · 10 comments
Assignees

Comments

@magsout
Copy link
Member

magsout commented Oct 22, 2015

I don't know if it's me, but each time config.py is updated (and of course I don't see it) I spend many times to understand the errors in my terminal.

Plus, with the @karlcow 's idea about export some information like the list of Categories (which is the great idea,) can may complicate maintenance of this file because it does not track by git. I wonder if it would not be better to have two files, maybe something like: config_app.py and config_access.py

  • config_access.py has only information about github acces (call_back_url, client_secret etc) is not tracked by git
  • config_app.py is exactly same as config.py but withtout variable (callback_url, secret key etc..) but import config_access.py to have access for callback_url, client_secret etc) is tracked by git. We can add information without break the config and the website.

It's just a thought. What do you think ?

@hallvors
Copy link
Contributor

Moving some secrets to config_access.py seems like a good idea, although AFAIK what broke local setup last time was that the variable name of one of the secrets changed. Also, looking through config.py as it is, it seems to have only a few things that are not secret..

@karlcow
Copy link
Member

karlcow commented Oct 22, 2015

It's an idea. Another one would be to check in run.py if the file if the local config.py (aka user's machine) is in sync with the config.py.example knowing that there are only a couple of secrets to ignore.

Another one is (a variation of @magsout proposal):

  1. to put config.py under tracking as it is except for the secret keys
  2. to import secrets.py at the top of config.py, secrets.py being not tracked (and having a tracked secrets.py.example)

@hallvors
Copy link
Contributor

A runtime check in run.py is a cool idea. It could relatively easily remove all strings (single- or doublequoted) from the config.py and config.py.example sources, then do a string comparison. It might have to take care of linebreaks. So run.py would output an error if the files didn't match, something like

Configuration change required - please check the latest changes affecting config.py.example and update your config.py accordingly.

@miketaylr
Copy link
Member

I have zero opinions on how we do this, except I have broken production because I haven't added some new config variables from config.py.example to my config.py (so of course the app explodes). So whatever we do, it'll be a good thing.

Lots of good options here, as long as we keep the environment variables we use for Travis (but those would likely stay in secret.py or whatever we end up with).

@magsout
Copy link
Member Author

magsout commented Oct 22, 2015

IMO, We just need to separate the environment variables (secret key, url callback) form the Constants or other necessary variables in runtime.

I do not know too Python framework, so it's hard for me to suggest something to normalize.

But I tend to repeat the same mistakes in that file and same errors.

@hallvors
Copy link
Contributor

We need to get more specific here. The first thing in config.py.example that's potentially secret is

# Secret key for signing cookies.
# Doesn't really matter for local testing.
SECRET_KEY = "meow"

How secret is this secret? Move to secrets.py?

GitHub client secret etc - this is secret and definitely moves to secrets.py:
https://github.com/webcompat/webcompat.com/blob/master/config.py.example#L66-L82
but the lines that read environment variables can stay in config.py

OAUTH_TOKEN="" should move to secrets.py but if it wasn't set there, config.py can try to read it from environment variables.

So, secrets.py.example might be https://gist.github.com/hallvors/fdcfe5aeaea86b0fcf1f

@miketaylr
Copy link
Member

How secret is this secret? Move to secrets.py?

Super secret. Needs to get moved, IMO.

The rest sounds good. 👍

@miketaylr
Copy link
Member

OK, I'm going to take the following approach:

Have a top-level config/ folder where we stick config-y things:

__init__.py to keep non-sensitive application stuff (this will replace config.py)
secrets.py for secret stuff (to be created from secrets.py.example)
secrets.py.example with empty values for people to fill in

In the future we can add labels.py and whatever else we want in the config/ folder, and import them from __init__.py

I think this should be a not-too-painful upgrade, because we'll keep config.py in gitignore, everyone will just have to manually copy over their secrets to secret.py just once.

miketaylr pushed a commit that referenced this issue Dec 29, 2015
miketaylr pushed a commit that referenced this issue Dec 29, 2015
As a result, we no longer need config.py(.example)
miketaylr pushed a commit that referenced this issue Dec 29, 2015
@miketaylr
Copy link
Member

Oops, need to split up b479fa1.

miketaylr pushed a commit that referenced this issue Dec 29, 2015
As a result, we no longer need config.py(.example)
miketaylr pushed a commit that referenced this issue Dec 29, 2015
miketaylr pushed a commit that referenced this issue Dec 29, 2015
@hallvors
Copy link
Contributor

👍

miketaylr pushed a commit that referenced this issue Dec 30, 2015
miketaylr pushed a commit that referenced this issue Dec 30, 2015
miketaylr pushed a commit that referenced this issue Dec 30, 2015
miketaylr pushed a commit that referenced this issue Dec 31, 2015
miketaylr pushed a commit that referenced this issue Dec 31, 2015
As a result, we no longer need config.py(.example)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants