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

Add configs #39

Merged
merged 4 commits into from Dec 19, 2015
Merged

Add configs #39

merged 4 commits into from Dec 19, 2015

Conversation

mattfullerton
Copy link
Contributor

Ability to use local file storage instead of s3
Ability to serve to only certain IPs (useful when using a test instance for embargoed data)

- Ability to use local file storage instead of s3
- Ability to serve to only certain IPs (useful when using a test instance for embargoed data)
@pudo
Copy link
Contributor

pudo commented Dec 14, 2015

I obviously love the file support, but the limit IP thing seems to me like something that should be done in a higher-level web server (nginx, apache). Are you sure we need it in core?

@mattfullerton
Copy link
Contributor Author

We absolutely do not :) But when quickly developing something on a home PC I have neither nginx, apache or AWS security groups (and this could be quite a common developer use case... embargoed data etc). Would you like me to prune down the PR?

@pudo
Copy link
Contributor

pudo commented Dec 14, 2015

wdyt, @pwalsh @milafrerichs ?

@pwalsh
Copy link
Member

pwalsh commented Dec 14, 2015

File support is excellent. Very much looking forward to getting this into OS.

As for the IP limit thing, I think it is fine here, and useful as @mattfullerton points out. I've personally done this type of thing on various django and flask code bases.

@breyten
Copy link

breyten commented Dec 14, 2015

+1 for file support. As for the IP settings maybe something like this:

allowed_ips = app.config.get('ALLOWED_IPS', [])
if len(allowed_ips) > 0 and request.remote_addr not in allowed_ips:
    abort(403)

This way you have one setting, instead of two. (But this is a bit of nitpicking ;))

@milafrerichs
Copy link
Contributor

File support is excellent. But I would rather see the settings made available via the environment.

e.g.:

STORAGE_TYPE = env('SPENDB_STORAGE_TYPE', 's3')
STORAGE_PATH = env('SPENDB_STORAGE_PATH', 'default/path')

not sure about the IP limiting...

@pudo
Copy link
Contributor

pudo commented Dec 17, 2015

Hm, so I really do feel like this would be better to keep apart and merge the storage, while keeping the IP filtering to front-end tools?

@mattfullerton
Copy link
Contributor Author

OK, I've removed IP support and added defaults/env variable loading as suggested by @milafrerichs

STORAGE_PATH = '/a/path/only/valid/if/using/local/file/system'

STORAGE_TYPE = env('SPENDB_STORAGE_TYPE', 's3') #Alternative: 'file'
STORAGE_PATH = env('SPENDB_STORAGE_PATH', '/usr/local/lib/spendb') #Only used if 'file' selected above
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to define it twice. can you remove the lines 22 & 23?

@milafrerichs
Copy link
Contributor

see the comment, otherwise looks great. thanks @mattfullerton

@mattfullerton
Copy link
Contributor Author

@milafrerichs that's what happens to commits at 7:23... removed the duplication

@pudo
Copy link
Contributor

pudo commented Dec 19, 2015

You guys rock :)

pudo added a commit that referenced this pull request Dec 19, 2015
@pudo pudo merged commit 1eac8e8 into openspending:master Dec 19, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants