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

Fix data_seed migration data being skipped #22860

Merged
merged 6 commits into from
Nov 22, 2024
Merged

Fix data_seed migration data being skipped #22860

merged 6 commits into from
Nov 22, 2024

Conversation

KevinMind
Copy link
Contributor

@KevinMind KevinMind commented Nov 15, 2024

Fixes: mozilla/addons#15153

Description

Makes sure if we are going to run data_seed in the initialize command that we do not pre migrate the database as this could lead to data being dropped post migration.

Additionally, use reset_db instead of flush as this will ensure the entire DB is dropped/created ensuring migrations actually run afterward.

Minor cleanup removing redundant uneeded code.

Context

When running data_seed there are 2 issues. 1) migrations have already been run by initialize this can lead to migrations that should run not being run again after flushing the db 2) flushing the db does not reset migrations so if they already ran, will not run again.

This can lead to a scenario where the database migrates data and then removes it, and the second migration does not reintroduce the data.

Testing

Run clean up

WARNING: this will wipe your current database state.

make up INIT_CLEAN=True

Now in a shell run

./manage waffle_switch --list

Expect this output including waffle switches generated during migrations.

run-action-in-auto-approve: off
enable-mad: off
allow-deleted-guid-reuse: off
dsa-job-technical-processing: off
dsa-abuse-reports-review: off
dsa-appeals-review: on
dsa-cinder-forwarded-review: off
enable-soft-blocking: off
suppressed-email: off
enable-taar: on
enable-manifest-normalization: off
enable-mv3-submissions: off
enable-git-extraction-cron: off
enable-subscriptions-for-promoted-addons: off
record-install-origins: off
enable-cinder-reviewer-tools-integration: off
enable-activity-log-attachments: off
disable-linter-xpi-autoclose: off
return-to-amo-for-all-listed: off
disable-bigquery: off

You can verify other data created during migrations like License.objects.count() should return 26 licenses.

Checklist

  • Add #ISSUENUM at the top of your PR to an existing open issue in the mozilla/addons repository.
  • Successfully verified the change locally.
  • The change is covered by automated tests, or otherwise indicated why doing so is unnecessary/impossible.
  • Add before and after screenshots (Only for changes that impact the UI).
  • Add or update relevant docs reflecting the changes made.

@KevinMind KevinMind changed the title Skip initial migration if seeding on initialize.py Fix data_seed migration data being skipped Nov 21, 2024
@KevinMind KevinMind requested review from a team and eviljeff and removed request for a team November 21, 2024 11:16
@KevinMind KevinMind marked this pull request as ready for review November 21, 2024 11:16
@@ -98,6 +99,26 @@ def static_check(app_configs, **kwargs):
return errors


@register(CustomTags.custom_setup)
def db_charset_check(app_configs, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

Not that this is a bad check to have, but I can't tell how it's connected to the issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we are now using reset_db we rely on django-extensions to create the Database. This ensures that when that happens, it has the correct charset.

call_command('migrate', '--noinput')

self.logger.info('Loading initial data...')
call_command('loaddata', 'initial.json')
call_command('import_prod_versions')
call_command('import_licenses')
Copy link
Member

Choose a reason for hiding this comment

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

Can you link to the pr/issue where we made this command redundant? (I thought Baku's work was still in progress)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@KevinMind this is the patch that added the line, 2 days ago. How is it redundant after 2 days - it's not even be deployed to stage yet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bakulf and I discovered the bug in parallel. His patch fixed the issue without needing to wait for my patch. But my patch fixes the underlying issue by making sure that data added during migrations is not erased.

These both are local dev patches that won't impact any prod like environment

@KevinMind KevinMind merged commit a2945aa into master Nov 22, 2024
31 checks passed
@KevinMind KevinMind deleted the addons-15153 branch November 22, 2024 17:58
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.

[Bug]: Migrations seem not to be applying data to database in during make up
2 participants