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

asadiqbal08/Move migrate_data stuff from custom migration file to management command #2389

Closed
wants to merge 4 commits into from

Conversation

asadiqbal08
Copy link
Contributor

Pre-Flight checklist

  • Testing
    • Code is tested
    • Changes have been manually tested

What are the relevant tickets?

fixes: #2348

What's this PR do?

As per some findings over the PR: Django Upgrade to 3.2 and referring to this comment. It seems, we should move the logic of migrate_data from custom migration file to Django management command.
Drawback is that, we have to update the migration files for dependencies manually whenever there is a model update in third party applications such as Wagtail.

How should this be manually tested?

Need to run the configure_wagtail command to incorporate migration stuff in wagtail.

@codecov-commenter
Copy link

Codecov Report

Merging #2389 (4101bb7) into master (b1da2fc) will increase coverage by 8.68%.
The diff coverage is n/a.

❗ Current head 4101bb7 differs from pull request most recent head c1b836d. Consider uploading reports for the commit c1b836d to get more accurate results

@@            Coverage Diff             @@
##           master    #2389      +/-   ##
==========================================
+ Coverage   88.29%   96.98%   +8.68%     
==========================================
  Files         330      156     -174     
  Lines       15102     4979   -10123     
  Branches     1704        0    -1704     
==========================================
- Hits        13335     4829    -8506     
+ Misses       1517      150    -1367     
+ Partials      250        0     -250     
Impacted Files Coverage Δ
...tic/js/components/forms/BulkEnrollmentForm_test.js 98.73% <0.00%> (-1.27%) ⬇️
cms/api.py
affiliate/admin.py
mail/constants.py
ecommerce/__init__.py
ecommerce/apps.py
voucher/utils.py
mitxpro/models.py
sheets/coupon_assign_api.py
courseware/models.py
... and 165 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b1da2fc...c1b836d. Read the comment docs.

@asadiqbal08
Copy link
Contributor Author

asadiqbal08 commented May 10, 2022

Today, I noticed an issue after moving the data-migration logic from migration file to manage command. After running some uniittests, it seems to me they depends upon migrate_data logic that was earlier in migration file. I am not sure, maybe that was the reason earlier to keep this stuff in migration files instead of moving under a management command.
So most of the unittests are failing due to this reason.

unittests expect to have page structures as per the logic and failed.

@asadiqbal08
Copy link
Contributor Author

@arslanashraf7 do you think can we proceed with this PR or need to close it. ?

cc @pdpinch

@arslanashraf7
Copy link
Contributor

@arslanashraf7 do you think can we proceed with this PR or need to close it. ?

cc @pdpinch

As per your comment i think of these options:

  • With your assessment do you think either running this command as is or converting it into a script that runs before our test step runs in the CI would help fix the tests?
  • If the above isn't feasible, and that might be the case since not only tests but the actual database gets updated when these migrations run on an existing instance like RC, we might end up breaking a few things that might take more time to fix later. We should definitely close the PR in this case.

I'll go with your assessment.

@asadiqbal08 asadiqbal08 deleted the asadiqbal08/issue-2348 branch July 21, 2022 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move migrate_data stuff from custom migration to management command
3 participants