Improve initial setup #668
Conversation
It had only the group id as a primary key, what would cause exceptions as soon as a record using the same group id was inserted. This wasn't detected in production because the tables were already there when the migration was created, but when starting a db from scratch it doesn't work well.
Using the sql files to initialize the db was conflicting with the alembic migrations.
anarute
force-pushed
the
improve-initial-setup
branch
from
5dbb426
to
cafdfe3
Compare
There was a problem hiding this comment.
Good changes here to simplify setup - thanks! I just had the one question related to what is/isn't optional in terms of initial data.
Another thought: now that we're using alembic for migrations and you have modified the docker-compose file, can we actually get rid of the test.db.Dockerfile? If yes, then we could probably also get rid of a few of the sql files we were copying into the container.
|
|
||
| `docker exec -ti phpreport-db alembic revision --autogenerate -m "Migrations description"` | ||
| [Optional] When running the project for the first time, you can add some |
There was a problem hiding this comment.
I'm not sure about marking this as optional. It feels to me that some of the data is a little less optional. The user groups are probably something we'd want to have folks add. And at least one user record is probably also recommended. The data from about line 52 on in the initialData.sql file seem like the most optional of the bunch.
In the migration for adding the task types table, you nicely added an insert statement for some default task types. I wonder if it would be worth updating the initial migration to add the bare metal data needed. Then modify the initialData.sql to include only the optional data (and perhaps rename to optionalInitialData.sql). What do you think?
There was a problem hiding this comment.
can we actually get rid of the test.db.Dockerfile?
Not yet, it's still being used for the automated tests: https://github.com/Igalia/phpreport/blob/main/.github/workflows/runtests.yml#L17
I'm not sure about marking this as optional. It feels to me that some of the data is a little less optional. The user groups are probably something we'd want to have folks add. And at least one user record is probably also recommended. The data from about line 52 on in the initialData.sql file seem like the most optional of the bunch.
In the migration for adding the task types table, you nicely added an insert statement for some default task types. I wonder if it would be worth updating the initial migration to add the bare metal data needed. Then modify the initialData.sql to include only the optional data (and perhaps rename to optionalInitialData.sql). What do you think?
I'm not sure about adding them to the migrations because even though they are needed for the app to work they should be configured per environment, I'm thinking on the case of setting up a production env for instance. Maybe what we want as a next improvement is a script to insert seed data for dev and production environments? I think only the user groups are more mandatory for any environment. I'm a bit concerned to add hard coded users for any environment. In the case of the task types I've added there because they were hardcoded in the front end, but maybe when we have a seed script it makes sense to also remove them from there?
There was a problem hiding this comment.
I like the idea of seed data based on environments as a next improvement and moving the Task Type data into one of those.
Several improvements on how we can setup the development environment using docker and a clean database.