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.

Sign up for GitHub

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

Jump to bottom

Convert ToolDeployment to a django model #1423

Merged
merged 25 commits into from
Jan 28, 2025
Merged

Convert ToolDeployment to a django model #1423

merged 25 commits into from
Jan 28, 2025

Conversation

michaeljcollinsuk
Copy link
Contributor

@michaeljcollinsuk michaeljcollinsuk commented Dec 27, 2024 •
edited
Loading

📝 Summary

This PR resolves ministryofjustice/analytical-platform#6266

This PR refactors how deployed tools are tracked and deployed. A database record will be stored for each time a user deploys a new tool. This will make it much easier for us to see how many users are using each version of a tool in the Control Panel.

It also allows us to refactor the frontend for listing and deploying tools, simplifying what details need to used on the tool list page and what to POST back when deploying a tool.

Merging this PR will have the following side-effects:

  • It will be difficult to rollback these changes because of the extend of changes made. Particularly because much of the information required to deploy a helm chart will now be read from the database.

🔍 What should the reviewer concentrate on?

  • Changes in the views
  • Changes to convert ToolDeployment to a django model
  • Changes to how tool deployments are triggered

🧑‍💻 How should the reviewer test these changes?

  • Run the migrations with python manage.py migrate
  • Rebuild the JS with make build-js
  • Alternatively rebuild your dev container, which should do run both the above as part of setup
  • Run the project locally, ensuring you are running the background worker. Celery is not required but doesnt hurt to have it running either
  • You will need some deployable Tool releases that you can use to test with, ideally multiple releases for each tool to test switching between versions. I consider adding some fixtures to create these, but it would expose some secrets that are stored in the values field. So instead, I would suggest copying tool releases from prod/dev. If taking from prod, if you want to try accessing the tool you will need to update some of the helm values for the release, such as the auth0 domain and client ID/secret. Let me know if you need a hand with this.
  • Go to the Your Tools page, select a tool, and deploy. This should work as before e.g. status updates to deploying, then deployed. She deployed tool should show as "Installed" in the drop down menu when it has successfully deployed
  • Switch between different release versions and check everything is working as expected.
  • Use the "restart tool" button to restart a tool. Again, this should work as before
  • When you have some tools deployed, go to the Tool Release admin page - there is now a column that indicates the number of users that are using each tool. Check the information displayed is correct.

📚 Documentation status

  • No changes to the documentation are required
  • This PR includes all relevant documentation
  • Documentation will be added in the future because ... (see issue #...)

@michaeljcollinsuk michaeljcollinsuk linked an issue Jan 20, 2025 that may be closed by this pull request
🔱 Track Tool deployments within Control Panel ministryofjustice/analytical-platform#6266
Closed
7 tasks
@michaeljcollinsuk michaeljcollinsuk force-pushed the spike/track-tool-deployment branch from f92b17e to 7207e77 Compare January 20, 2025 16:00
jamesstottmoj
jamesstottmoj reviewed Jan 24, 2025
View reviewed changes
Copy link
Contributor

@jamesstottmoj jamesstottmoj left a comment

Choose a reason for hiding this comment

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

Manually tested and seems to work ok.

One issue I ran into was the caching of javascript code which meant no buttons would activate on the tools page so that may be something to potentially be aware of (hard refresh gets round this).

Potential idea for making use of this data is to automatically uninstall tool releases that are marked as retired. This will force users to use the control panel to install the latest release of that tool.

@michaeljcollinsuk
Convert ToolDeployment to a django model
4c9ddaa 
Allows us to track the specific tool release that
a user has deployed.
This commit includes minimal changes to get the
model working, so that every time a tool is
deployed from the front end, a database object is
created. This does introduce a bug about status of
the tool deployment, but this will be fixed later.
@michaeljcollinsuk
Track if a deployment is active
3dcf2ae 
Use an is_active field to allow us to track past
deployments.
This also makes it easier to uninstall an old tool
before we install a new one.

NB a bug will occur if a user has no active
ToolDeployment but tries to switch from a jupyter
datascience to all-spark release (or vice versa).
To resolve, we have to manually install the users
Jupyter release, or deploy the same type of
Jupyter to create an "active" ToolDeployment
object, and then the user can deploy the other
type of Jupyter.
@michaeljcollinsuk
Simplify tool list view logic
c4deaf4 
Deletes a lot of code that was used to display
the deployed tool, to read information from the
tool models where possible.
Further refactoring will be needed to add a
form to handle tool choices.
@michaeljcollinsuk
WIP replace tool options with forms
9ad5fb9
@michaeljcollinsuk
WIP Fix tool options display
bb1b3fc
@michaeljcollinsuk
Update tool deploy api view
b98dfd5 
Simplify the logic to deploy tools based on the
ID. Update ToolDeployment serializer
@michaeljcollinsuk
Some cleanup
757c1e4
@michaeljcollinsuk
Update tool deployment field name
87d2ee2
@michaeljcollinsuk
Refactor the tool views, remove unneeded code
3e15ab5
@michaeljcollinsuk
Refactor building airflow urls
9f5a738
@michaeljcollinsuk
Refactor serializer to create a ToolDeployment obj
320efe3
@michaeljcollinsuk
Remove id_token from update_tool_status
741048d
@michaeljcollinsuk
Fix some bugs, add ordering
0bd4103
@michaeljcollinsuk
Update form display
1815e8e
@michaeljcollinsuk
Display number of users on tool release list page
690acf2
@michaeljcollinsuk
Fix some tests
090ac62
@michaeljcollinsuk
Rebuild migrations
b4f20bf
@michaeljcollinsuk
Fix more tests
e0b0469
@michaeljcollinsuk
Rebuild migration after rebase
899967d
@michaeljcollinsuk
Update tool restart implementation
3e6bbdd
@michaeljcollinsuk
Refactor RestartTool view to use a form
508e33d
@michaeljcollinsuk
Revert to using a redirect view
8e08316
@michaeljcollinsuk
Add management command to create ToolDeployments
9a31df1 
Loops through all namespaces, and creates a DB
record to correspond with the tool that they have
deployed.
@michaeljcollinsuk michaeljcollinsuk force-pushed the spike/track-tool-deployment branch from 9092bfa to 9a31df1 Compare January 24, 2025 17:44
jamesstottmoj
jamesstottmoj previously approved these changes Jan 27, 2025
View reviewed changes
Copy link
Contributor

@jamesstottmoj jamesstottmoj left a comment

Choose a reason for hiding this comment

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

LGTM

@michaeljcollinsuk michaeljcollinsuk marked this pull request as ready for review January 27, 2025 16:57
@michaeljcollinsuk michaeljcollinsuk merged commit a70b444 into main Jan 28, 2025
12 checks passed
@michaeljcollinsuk michaeljcollinsuk deleted the spike/track-tool-deployment branch January 28, 2025 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jamesstottmoj jamesstottmoj jamesstottmoj approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

🔱 Track Tool deployments within Control Panel
2 participants
@michaeljcollinsuk @jamesstottmoj