codebuddies / backend

CodeBuddies back-end
https://codebuddies.org
GNU General Public License v3.0
20 stars 25 forks source link

Document the use of the makemigrations command #70

Closed lpatmo closed 4 years ago

lpatmo commented 4 years ago

Summary

I want to run pip install -r requirements/local.txt while inside the backend_app docker container so that I can install the taggit_serializer module, but I can't get inside it because my containers are continuously restarting.

Recreation of steps

$ docker-compose down $ docker-compose rm $ docker-compose up (with no -d)

Result: image

From those error messages, I saw that a module taggit_serializer was not installed.

In a non-docker environment, normally I would pip install -r requirements/local.txt (or `pip install -r requirements.txt assuming that defaults to running the local.txt file which is specific to cookiecutter) while I am inside a virtualenv.

But because I am running docker, I'll have to run that command inside a container.

From https://stackoverflow.com/questions/56904783/how-to-install-a-python-library-to-a-pulled-docker-image, I found that I could run:

$ docker ps #check for the container ID $ docker exec -it container_id bash

So I tried this: image

But I keep getting the message that the container is restarting on the backend_manage and backend_app containers.

image

Am looking at https://stackoverflow.com/questions/37471929/docker-container-keeps-on-restarting-again-on-again next for hints on what I can do to solve the "container is always restarting" problem 😅

billglover commented 4 years ago

Will add more detail later...

docker-compose build

I think the following may also work but need to check.

docker-compose up --build

The trick is knowing when this needs to be run. That’s where I get stuck. We could say run it all the time, we could say run it only if certain things change, or we could create a Makefile (Windows users may miss out here).

lpatmo commented 4 years ago

Ahhh thank you so much, docker-compose up --build worked!! :D I noticed it also applied migrations; do you know if it also does makemigrations?

The trick is knowing when this needs to be run.

Generally, we need to pip install -r requirements/local.txt whenever a new Python module is added. And we need to makemigrations and migrate each time the DB model changes.

So maybe the tip would be to run it after each git pull from the master branch, in case there have been model changes or new modules added? (hand-wavy)

billglover commented 4 years ago

Because we are including dependencies in the container image, the image needs to be rebuilt whenever these dependencies change. You can see exactly what gets run here:

https://github.com/codebuddies/backend/blob/9123fabe517ab65aae0e3d360c2121fdc24f4267/cbv3_django_prototype/Dockerfile#L7-L10

Migrations are run when the application is started. The script can be seen here:

https://github.com/codebuddies/backend/blob/9123fabe517ab65aae0e3d360c2121fdc24f4267/docker-compose.yaml#L33-L34

I'm still learning the Django development workflow but I don't think we want to run make migrations at run-time. I think this should be run and the resulting migrations included in any commit that requires migrations.

BethanyG commented 4 years ago

makemigrations should be run only by someone who has made a model (db) change.

If they've made

They should be double checking their migration files and consolidating, organizing, and deleting excess where necessary.

Only the migration files that make the final intended changes should be checked in (e.g. the final field name in its migration, the final PK change in its migration)- intermediary back and forth files can create confusion and mess.

..and if complicated changes have been checked in as part of a PR then (unlike what I did...and duly noted for the next time) the PR should be flagged as one where a re-build is needed.

BethanyG commented 4 years ago

I also think that dependency changes should be called out in the PR descriptions and flagged with a rebuild flag as well. I wonder if we can make a PR template that prompts that??

billglover commented 4 years ago

Turning this into actionable tasks, I think for now we need:

  1. Document guidance for PR reviewers to check that model changes are accompanied by associated migrations. Suggested location on wiki
  2. Document guidance for contributors to ensure they are running makemigrations at an appropriate point. Suggested location on wiki.

Both should be documented on the wiki in the sections linked above.

sunu commented 4 years ago

I would propose instead of marking changes for rebuild, we always do docker-compose build before doing docker-compose up. Docker build layers are cached and if nothing has changed, build process finishes in no time since there is nothing new to build and every required layer is already available in cache.

billglover commented 4 years ago

I'm all for reducing the number of decisions people need to make. We still need documentation on makemigrations, so our actionable tasks become:

  1. Document guidance for PR reviewers to check that model changes are accompanied by associated migrations. Suggested location on wiki
  2. Document guidance for contributors to ensure they are running makemigrations at an appropriate point. Suggested location on wiki.
  3. Update the README (setup step 4) to include the --build flag to ensure dependencies are updated if necessary.
bengineerdavis commented 4 years ago

I won't be able to work on this issue, as planned, please un-assign me, and I will look into this at a later date when circumstances allow. Thank you!

billglover commented 4 years ago

@bengineerdavis no problem at all, thanks for letting us know.

BethanyG commented 4 years ago

@lpatmo @billglover -- is this still something we need open?

lpatmo commented 4 years ago

Because there's a lot to read through in this thread (including an OP post by me that isn't entirely relevant), I think it may be confusing for someone newly coming across this task.

I think Bill's acceptance criteria above still makes sense to document, but maybe in the README.md (where the most-up-to-date contribution instructions live at the moment) instead of on the wiki... will file a clean issue for this.