openoakland / OakCrime-Decommissioned

Code supporting citizen analysis of crime in Oakland, CA
22 stars 16 forks source link

Cleaned up environment variables in harvestPatrolLog.py #51

Closed clintonb closed 5 years ago

clintonb commented 5 years ago

The environment variables are now loaded as settings and referenced as such from the management command. This consolidates all consumed environment variables into a single location (settings) to aid new developers and prevent surprises when creating a new instance.

Suggestion: Hide whitespace when viewing the diff. It's not as bad as the line count makes it look. See https://github.blog/2018-05-01-ignore-white-space-in-code-review/.

clintonb commented 5 years ago

I don't exactly know how any of these are used. I'm just trying to do basic cleanup and organization of settings. We can file a ticket for updating docs, but I don't think that should prevent this from being merged.

adborden commented 5 years ago

Right, my concern with the variables is that without a default set (or it being documented), any developer will start the app and get an error because of missing settings. I'm just trying to avoid people not knowing how to start the app. Even something like this would be fine:

These environment variables are required for development although their values are not significant for general development. You can create a .env file and source it before running the application. TODO: document each variable, what it means, and where to find a correct value.

.env

export GoogleAPIKey=unset export BoxAPIKey=unset

etc...

-- Aaron D Borden Human and hacker

clintonb commented 5 years ago

These settings are used by an undocumented management command. They are not needed to start the service. I agree they should be documented when the management command is documented.

rbelew commented 5 years ago

doc for harvestPatrolLog command

adborden commented 5 years ago

@clintonb I must be misunderstanding something. Should I expect the app to start with your changes even if I'm not running the harvestPatrolLog job?

$ python manage.py runserver
Traceback (most recent call last):
  File "/home/adborden/.pyenv/versions/OakCrime/lib/python3.6/site-packages/environ/environ.py", line 273, in get_value
    value = self.ENVIRON[var]
  File "/home/adborden/.pyenv/versions/3.6.8/lib/python3.6/os.py", line 669, in __getitem__
    raise KeyError(key) from None
KeyError: 'BoxEnterpriseID'

It looks to me like these settings are required to start the app.

clintonb commented 5 years ago

Good catch. Those should default to None.

adborden commented 5 years ago

Replaced by #71