Remove initialize_docker: Integrate data initialization to make up #22663
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
KevinMind
force-pushed
the
clean-init
branch
15 times, most recently
from
0810482
to
5737991
Compare
KevinMind
requested review from
a team,
eviljeff and
diox
and removed request for
a team
KevinMind
force-pushed
the
clean-init
branch
5 times, most recently
from
44d0a4f
to
6b9dbdc
Compare
eviljeff
reviewed
Sep 19, 2024
|
I tried following the testing steps and seemed to be a state where the script thought I had a database ("Database already exists") and needed to run all the migrations, which seems wrong. log |
|
@eviljeff This looks like expected behavior. If you follow the tests and the code the migrations always run. I'd be curious what happens if you run make up again. It should simply say all migrations have been applied. Look at the test cases for more sophisticated scenarios. |
I'm trying to test the scenario of a clean install - that all the migrations had to run indicates the database was empty, but initialize_data seemingly didn't do that creation, because the logging indicates the database already existed at that point. |
Can you show me exactly what you ran to do a fresh install? For that you'd have to drop the mysql volume and run make up. or run with It looks like the database existsed, but was not migrated in this case. I'm not totally sure how that happened so I would need more context on exactly what you ran. |
I'm very glad you caught that. That was a bug and in squashing it I realized that this is still a lot more complicated than it needs to be. I'm going to switch back to draft and will re-request review once I've updated the code/tests. |
5 tasks
KevinMind
force-pushed
the
clean-init
branch
10 times, most recently
from
66c6a6c
to
cbb9f0d
Compare
KevinMind
force-pushed
the
clean-init
branch
2 times, most recently
from
99e4844
to
a4b5460
Compare
That didn't work for me, reindex did not occur, and I was left with a broken frontend. I can see these initialization steps (but no reindex) in the output: |
diox
reviewed
Oct 7, 2024
@diox this commit fixes that issue. Was introduced by a different PR that landed in between this one opening and now. |
Rename "initialize_data" to "initialize" Remove DOCKER_SERVICES as dangerous option given hard dependency on mysqld and elasticsearch. Better timing for mysql healthcheck
diox
approved these changes
Oct 11, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: mozilla/addons#15020
Description
Integrate data initialization to make up by integrating the 3 types of data initiailization commands to management commands, making them idempotent, and hooking them into make up.
With this, we no longer need
initialize_dockeras make up will do everything it does if the project is pristine.Context
We haven't really needed the initiailzation command for a while, but now that everyone is fairly used to rely on make up for "doing everything" we should make it actually do everything.
This approach integrates data initialization as another part of the make up pipeline. What is mostly different here is that now make up will ensure:
This has a pleasant side effect that CI now verifies database migrations more explicitly. We could consider migrating our testDB to use the regular DB in CI, but that is outside of this PRs scope.
Testing
You should run
make upin the following scenarios.make down && make docker_mysqld_volume_remove).make upto start from a "clean" data perspective. Expect: create DB, seed DB, index.make upagain with running container and seeded db Expect: it does nothingmake down && make upExpect: It should skip seeding and only reindex the DB.make shelland run./manage.py flush --noinput. Then runmake down && make upExpect: it should reseed DB and reindex again.Expect the flags to work as expected:
INIT_CLEAN: will wipe DB and index and reseed and reindex regardless of the existing DB stateINIT_LOAD: will load the given snapshot name and reindex.DATA_BACKUP_SKIP: This will skip the initialize command entirely. No migration/seed/load/reindex, nothing.The command itself is tested for various scenarios specified above. But you can manually inspect the commands run as the tests do not test the results of the commands but the execution order.
Checklist
#ISSUENUMat the top of your PR to an existing open issue in the mozilla/addons repository.