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

chore: Automate collectstatic and migrate commands #427

Merged
merged 6 commits into from
Apr 23, 2024

Conversation

microamp
Copy link
Contributor

Fixes #426


deploy.sh already exists, so it's being reused here.

cat deploy.sh
#!/bin/sh -e

cd $(dirname $(echo $0))

python manage.py collectstatic --no-input
python manage.py migrate --no-input
python manage.py update_index

We can expand the list if there are any commands missing that need to run on init.

@microamp microamp requested a review from kesara April 19, 2024 03:28
@codecov-commenter
Copy link

codecov-commenter commented Apr 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.80%. Comparing base (c80251d) to head (3a4ee86).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #427      +/-   ##
==========================================
+ Coverage   82.72%   82.80%   +0.07%     
==========================================
  Files         124      126       +2     
  Lines        2599     2611      +12     
==========================================
+ Hits         2150     2162      +12     
  Misses        449      449              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mgax
Copy link
Contributor

mgax commented Apr 19, 2024

Running commands on Docker startup is not necessarily a bad thing, for example it's a common practice to run migrations like this, but:

  • collectstatic should really be run as part of the Docker image building, all its input is already present in the image.
  • update_index should be run regularly, e.g. once a week. It takes a while to run, depending on the number of objects in the database. It's not something you want to run when you're failing over to a new Docker container for whatever reason.

@microamp
Copy link
Contributor Author

@mgax

Thanks for your input, much appreciated!

Re: collectstatic, I agree. I remember trying that earlier, but it wasn't working for some reason. I'll take another look.

Re: update_index, it's fairly quick to run against the IAB database at least, but I agree that it needs to be run on a regular basis. Is Celery the go-to option here, or something potentially more lightweight?

@mgax
Copy link
Contributor

mgax commented Apr 19, 2024

Thanks for your input, much appreciated!

Any time, hope it helps :)

but I agree that it needs to be run on a regular basis. Is Celery the go-to option here, or something potentially more lightweight?

I think the simple option is some kind of cron mechanism. I've noticed there's a new Helm deployment being set up, and I don't know much about Helm/Kubernetes, but it looks like Kubernetes has the concept of cron jobs?

@rjsparks
Copy link
Member

There are a number of other management commands that are recommended to be run periodically at https://docs.wagtail.org/en/stable/reference/management_commands.html

@microamp microamp changed the title chore: Automate management commands on init chore: Automate collectstatic on init Apr 22, 2024
@microamp
Copy link
Contributor Author

I've updated the init script so that it only runs collectstatic. When tested locally, it takes about a second to complete. Maybe good enough as an interim solution for now. Let me know otherwise.

root@www-wagtail-75fd4c8495-k7kp4:/app# time python manage.py collectstatic --no-input
0 static files copied to '/app/static', 309 unmodified, 186 post-processed.

real    0m1.042s
user    0m0.824s
sys     0m0.215s

@mgax I have looked into including static files in the image itself, but it expects some environment variables such as APP_SECRET_KEY and DATABASE_URL to be there. If they are undefined, the docker buildx build command would fail. I can probably do something like this, but I'm not sure if that's the best option we have. Please let me know if I'm reading it wrong.

@mgax
Copy link
Contributor

mgax commented Apr 22, 2024

I have looked into including static files in the image itself, but it expects some environment variables such as APP_SECRET_KEY and DATABASE_URL to be there. If they are undefined, the docker buildx build command would fail. I can probably do something like this, but I'm not sure if that's the best option we have.

@microamp yep, setting placeholder values for the required environment variables in Dockerfile RUN steps is a common practice. The collectstatic command doesn't need access to a database, or a secret key, but it needs to bootstrap the Django project, and the project's settings do require the environment variables.

I've updated the init script so that it only runs collectstatic.

I'd also include ./manage.py migrate in there, unless it's being run somewhere else.

@microamp
Copy link
Contributor Author

microamp commented Apr 22, 2024

@mgax Thanks for your input once again.

I will try to build and include static files in the image itself.

I'd also include ./manage.py migrate in there, unless it's being run somewhere else.

Thanks for pointing that out. That is right - it needs to be part of the init process. I'll include that in init.sh again.

EDIT: typo

@microamp microamp changed the title chore: Automate collectstatic on init chore: Automate collectstatic and migrate commands Apr 22, 2024
@microamp microamp merged commit 8db6141 into ietf-tools:main Apr 23, 2024
5 checks passed
@microamp microamp deleted the chore/automate-mgmt-cmds branch April 23, 2024 04:18
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.

Automate collectstatic and migrate commands
5 participants