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

Pin all our direct dependencies #838

Merged
merged 1 commit into from Mar 12, 2024
Merged

Conversation

mbarton
Copy link
Member

@mbarton mbarton commented Mar 11, 2024

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:

Library Dependency Path Notes
Django django-simple-captcha > django-ranged-response We'll always have Django as a pinned main dependency. There has not been a release of django-ranged-response since 2017 but django-simple-captcha is well maintained so would likely address any breakage across major versions for us.
pypng django-two-factor-auth > django-formtools > qrcode No release since 2022 but there is activity in their source (https://gitlab.com/drj11/pypng). They specifically do not follow semantic versioning https://gitlab.com/drj11/pypng/-/commit/81809f032cdc334bab9893e4982470f9246efe14#5b84fdee5cb197a7299f979e9e9908e925ac4d0f_17_27. Again, breakage would likely be handled by django-formtools which had two releases last year so is fairly maintained.
typing_extensions django-two-factor-auth > django-formtools > qrcode This package exists purely to adopt typing in older Python versions and essentially does nothing on 3.11 (our version). They are hesitent to break backwards compat https://typing-extensions.readthedocs.io/en/latest/#versioning-and-backwards-compatibility
packaging black gunicorn plotly pytest-django > pytest pytest-factoryboy pytest-factoryboy > pytest Very widely adopted. Only one breaking change in the entire release notes and it was a technicality: pypa/packaging#734.
iniconfig pytest-django > pytest pytest-factoryboy > pytest One breaking change from Jan last year, otherwise no releases since Oct 20. Seems to follow at least major version semantic versioning?
pytz djangorestframework Project is in maintenance mode, apparently core Python >= 3.9 has enough functionality not to need this package (https://pypi.org/project/pytz/). Eventually djangorestframework will update to a newer base Python version and not need this any more As the project is in maintenance mode they are unlikely to introduce breaking changes.
inflection pytest-factoryboy No release since 2020 although there were some commits last year. Just a testing dependency though so at worst we would comment out the tests until fixed.

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
@mbarton mbarton self-assigned this Mar 11, 2024

# versioning
bump2version
Copy link
Member Author

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?

Copy link
Member

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.

@eatyourpeas
Copy link
Member

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:

  1. simple-history: we are using this as an audit trail of who touched what. It is customisable but we have not done this. It creates a history model for every model in the application and tracks who touched what and i think it is even possible to roll back if you need to. Again, this is not really used but was felt a requirement. I built our own user logs because I did not trust it enough for that, so for the simple process of tracking who logged in and out and at what times and from where, we use django signals and store the result in the VisitActivity table, which pulls into the UI for those with the correct permission. It is actively maintained (last commit 3 weeks ago) and is another Jazzband package.
  2. fuzzy logic. This was also a bit too niche to build ourselves. The multiaxial diagnosis seizure description section is the only place where free text is gathered, and we wanted to use the text imaginatively otherwise the data would be gathered and never used. I tried out several ideas in Vue and posted them on Twitter (back when Twitter was actually a thing) as a way of categorising the events of a seizure (what the carer or clinician witnessed) and mostly people liked the idea of the app collecting key words automatically as you type a freeform description of the 'semiology' of the episode, and posting them below the text. there was also a less popular drag and drop key words option. To allow a degree of spelling error we needed a fuzzy logic algorithm and after searching a lot, went with rapidfuzz.
  3. django-semantic-admin - this to me is the weakest dependency we have and I would not mind losing it. It was introduced really to make admin look nicer but i don't think it has done that. And admin does not need to look nice tbh so I think that is ripe for removal.
  4. django-widget-tweaks - this is another Jazzband project is meant to improve styling in django forms. I put it in before we had chosen to go with htmx. We have stuck with the standard django form with the old fill and submit, using form classes and so on in only 2 places: user creation and case creation. Those forms, being django forms were harder to add some of the semantic style classes to and in the end I think although this dependency was added, it is not actually used. So this one potentially could be removed, but we would need to do some investigating first.
  5. django_filters - this is used with drf. Although the DRF routes have been commented out, the DRF is very much in there and I would be loath to remove it though not used. This is one to discuss. I spent much of last summer while on a beach in Bournemouth laying down the routes. It does not have authentication and it is not clear who would use it yet, so currently lying dormant and unused.
  6. django-autologout - this I am also not averse to removing but we would need to build our own if we did. Last commit on the repo was 2022 and actually I am not sure we have tested it since the new django 4.2 requirement for logout to be a post request. Possibly deserves an issue of its own to explore further
  7. colorlog - @anchit-chandran added this one. I think it is a nice simple way to get colours in the console, is low maintenance and looks pretty solid. Last commit was in January, has ~850 stars on github and forked 86 times. I guess that last stat could either mean it is so good people are pimping it and contributing to its wellbeing, or it is shit and people don't want to start from nothing. I am betting on option 1.

Other dependencies

We have other dependencies in the browser.

  1. htmx - obviously this one is integral to the whole application. It has some django middleware which is pip installed which interrogates the headers for htmx attributes. The two play well together. The htmx library is a few hundred lines of minified javascript and for security reasons partly we pull it from disk, rather than CDN. It is therefore a dependency that we have actively to maintain in the same way as the others above.
  2. hyperscript - this is a separate project by the same guy who brought us htmx, Carson Gross. It is a nice way of manipulating the DOM without needing javascript. It has its own syntax which is quite easy to pick up and means you can write what you want to happen inline within html. In the same way, it is stored in the assets folder, so would need maintaining.
  3. semantic-ui - we have talked about this elsewhere. It is an abandoned project but which lives on in open-source as Fomantic and is actively maintained by an enthusiastic followership. I have used it for years which is partly why we have ended up with it, but agree it is not really the right tool for the job here. To use the theming properly ideally it should be npm installed and override the styles in the config, but we saved only the JS and CSS files in assets and therefore had to override the individual styles in a styles.css old-fashioned way so have not really had any of that benefit from it. It depends on jquery which our pentesters told us was amber and we should get rid of if we could.
  4. jquery - see above. If we were to lose semantic, then we could lose this too and I would be delighted. Any small bits of javascript that we might need could probably be done with hyperscript in any case.
  5. leaflet - this is a well-known solution for mapping on the front end, is maintained by a Ukrainian company and has been actively used in mapping during the war. We used it for the maps on the organisation page using base maps from carto. My longer term plan had been to remove this and find a way of doing it with plotly or some other backend gis mapping service and pass the images down the wire, as we have done with the graphs. @anchit-chandran instrumental here for the use of plotly here.
  6. chartjs - this is loaded from CDN. I think was added originally for the pie charts prior to using plotly. Could probably be removed easily with some investigation
  7. jquery-mask - this is a lazy dependency and could be refactored for sure. This masks the NHS number into a 4/3/3 format. Could not find an easy way to do this using django template tags but probably just did not think hard enough about it.
  8. bootstrap - this was added for theming the drf API. DRF use bootstrap and it was easier this way. Again very happy to remove.
  9. sweetalert - this is a fairly popular simple JS library that does modals. It is a nice easy way to generate popup modals as confirmation alerts. I could not quite get semantic to do this triggered by a button click without writing jquery and I was keen to avoid this. Again, would probably not be need with a better frontend css framework choice.

Sorry for the long reply. I think that is a fairly comprehensive list and certainly things that could be updated/improved/refactored.

@mbarton
Copy link
Member Author

mbarton commented Mar 12, 2024

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.

@mbarton mbarton merged commit abfffbc into development Mar 12, 2024
2 checks passed
@mbarton mbarton deleted the mbarton/freeze-dependencies branch March 12, 2024 10:06
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.

Freeze dependencies
2 participants