-
Notifications
You must be signed in to change notification settings - Fork 5
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
Pin all our direct dependencies #838
Conversation
pip freeze outputs pinned versions for all packages, including those we depend on transitively. I think upgrading our packages will be unnecessarily complicated freezing every time so I have copy pasted across the versions for the packages we depend on directly. I've also removed a few packages we are not using as far as I know
|
||
# versioning | ||
bump2version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we used this at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i have been using bump2version - though mostly it is used in the growth chart python package. it allows you to bump the version as a patch, major or minor version from the command line and also creates a tag.
We are not creating releases of the E12 so this has been less of a thing so I don't feel strongly about this.
This looks good to me. I think we have been generally quite careful with our dependencies. The two factor package has had its problems, and in particular overriding the styling is not very easy - the OTP input for example could not be styled the same as the rest of the the app, for example, nor could the radio buttons when choosing between email and authenticator app. At least, I expect we could identify the styles in the browser console, but there was no documentation and it all seemed too hard for such little benefit. I was persuaded by the package because it is part of jazzband who describe themselves as a django collaborative and they have a huge number of packages that they maintain and it sounded suitably responsible and thought through. We did have some difficulties with it (in relation to docker i think but i forget what) and @pacharanero posted an issue on the repo. Not sure if it was answered. If there is a better alternative I am all ears but it is the sort of thing that would take ages to build yourself and would not be that rewarding. It has more function that we need, but we do need 2FA (particularly after reading about the experience of the British Library..) There are a few others off the top of my head not on this list:
Other dependenciesWe have other dependencies in the browser.
Sorry for the long reply. I think that is a fairly comprehensive list and certainly things that could be updated/improved/refactored. |
That's very very useful context, thanks @eatyourpeas! I think it makes sense to address dependencies as we modify the parts of our codebase that use them, unless there's a critical issue. |
Fixes #833
We had an issue where our unpinned dependencies brought in a new Django major version automatically, causing the logout button to break (#837).
This PR pins all our direct dependencies to try and avoid this happening in the future. This naturally puts the emphasis on us to monitor and upgrade our dependencies as quickly as we can, in collaboration with our good friend Dependabot :)
I've deliberately chosen not to copy paste in the result of
pip freeze
as that would lock all of our transitive dependencies too and make it difficult to simply bump versions in the future without "unfreezing" and freezing again.I wanted to double check this approach hadn't brought across anything too scary transitively, especially our dependencies themselves having unpinned dependencies. I ran pipdeptree to get the full dependency tree and searched for
Any
.There were some documentation transitive dependencies (
PyYAML
,GitPython
) but I'm not worried - if they upgrade and cause issues we can easily split off the documentation to a separate build to get the app deployable again.For the main app these are the unpinned transitive dependencies. For each one I've added a little bit of context as to why I think it's ok not to worry right now:
django-simple-captcha > django-ranged-response
django-two-factor-auth > django-formtools > qrcode
django-two-factor-auth > django-formtools > qrcode
black
gunicorn
plotly
pytest-django > pytest
pytest-factoryboy
pytest-factoryboy > pytest
pytest-django > pytest
pytest-factoryboy > pytest
djangorestframework
pytest-factoryboy