ThreeSixtyGiving / datagetter

Scripts to download data from http://registry.threesixtygiving.org
MIT License
1 stars 1 forks source link

Setup: consider opening requirements.in not requirements.txt #22

Open odscjames opened 3 years ago

odscjames commented 3 years ago

I have a situation trying to update requirements in data-store app. urllib3 was not updating, and it needs to, because it's a security requirement. With the help of pip-compile's great --verbose mode I could work out it's because datagetter is a dependency of data-store but because datagetter setup.py loads requirements from requirements.txt it was locking urllib3 to the old version! I'll have to update reqs in datagetter first then go back and update reqs in data-store .

Things which are used as libraries shouldn't really have locked dependencies

Or if we can't fix this, can we at least make sure that every 360 app that uses this has a "Updating requirements" section that reads "you must update reqs in https://github.com/ThreeSixtyGiving/datagetter first!" or something?

michaelwood commented 3 years ago

the datagetter isn't used as a library that I know of, but if using requirements.in fixes that then that sounds good

odscjames commented 3 years ago

the datagetter isn't used as a library that I know of

Datastore uses it as a library?

michaelwood commented 3 years ago

the datagetter isn't used as a library that I know of

Datastore uses it as a library?

It is just there to install the executable so that the orchestrating shell script can access it. Technically it could be in it's own virtualenv but that adds a bit more complexity.

odscjames commented 3 years ago

Hmm. Well, the effect of it being in https://github.com/ThreeSixtyGiving/datastore/blob/master/requirements.in#L1 etc is that it is technically used as a library, even if no Python is imported, and that causes problems for requirements in datastore.

That's interesting tho - it means one possible solution here is to remove https://github.com/ThreeSixtyGiving/datastore/blob/master/requirements.in#L1 etc and set up another virtualenv instead. It is a touch more complex, but it's copy and paste salt to do that and we remove any complexities of the Python dependencies so this may come out as a win?

In we did that, does this repo even need a setup.py at all?

michaelwood commented 3 years ago

Thinking about it more, multiple virtualenvs would work but we would need to refactor the data runner script to flip between the virtualenvs a lot https://github.com/ThreeSixtyGiving/datastore/blob/master/tools/data_run.sh#L39

Happy to do your original suggestion of requirements.in

odscjames commented 3 years ago

Ok, now I know more about the context let me think. Having unlocked dependencies in python packages does come with more potential issues, but if it's a library you just have to deal with those issues. So I'm not sure which the best solution is. Talk about this soon, maybe?