Check any migrations change #33636
Check any migrations change #33636
Conversation
author Awais Qureshi <[email protected]> 1698834786 +0500 committer Awais Qureshi <[email protected]> 1701168550 +0500 build: capturing added or remove migrations. build: capturing added or remove migrations. build: capturing added or remove migrations. build: capturing added or remove migrations. build: capturing added or remove migrations. build: upgrading pillow. chore: fixing test. chore: minor change. chore: minor change. feat: Upgrade Python dependency social-auth-app-django (#33826) Commit generated by workflow `openedx/edx-platform/.github/workflows/upgrade-one-python-dependency.yml@refs/heads/master` Co-authored-by: awais786 <[email protected]> chore: minor change.
awais786
force-pushed
the
check-migrations
branch
from
4b764b7 to
8ce599f
Compare
|
Migrations added in this PR.
|
| run: | | ||
| pip freeze | ||
|
|
||
| - name: Run Tests |
There was a problem hiding this comment.
Should this be renamed, since it doesn't actually run any tests?
| STUDIO_CFG: lms/envs/minimal.yml | ||
| run: | | ||
| echo "Running the LMS migrations." | ||
| ./manage.py lms makemigrations |
There was a problem hiding this comment.
Is this here to catch any code changes which should create migrations, but the author forgot to add them?
| diff=$((number2 - number1)) | ||
| if [ $number2 -gt $number1 ]; then | ||
| data=$(mysql -h 127.0.0.1 -uedxapp001 -ppassword -e "select name from django_migrations ORDER by -id limit $diff;" edxapp;) | ||
| echo "$data" > cleaned_data.txt |
There was a problem hiding this comment.
I don't see the "Migrations added in this PR." text that appeared in the comment, did I miss that or was it removed over the course of code edits?
| number1=${{ env.FIRST_QUERY }} | ||
| diff=$((number2 - number1)) | ||
| if [ $number2 -gt $number1 ]; then | ||
| data=$(mysql -h 127.0.0.1 -uedxapp001 -ppassword -e "select name from django_migrations ORDER by -id limit $diff;" edxapp;) |
There was a problem hiding this comment.
We should also select and display app_label so it's clear where each migration came from.
|
|
|
All changes done. |
| diff=$((number2 - number1)) | ||
| if [ $number2 -gt $number1 ]; then | ||
| data=$(mysql -h 127.0.0.1 -uedxapp001 -ppassword -e "select app, name from django_migrations ORDER by -id limit $diff;" edxapp;) | ||
| echo "$data" > cleaned_data.txt |
There was a problem hiding this comment.
Adding to what Jeremy said earlier. I appreciate the app name and tables being added to the output, but I think it would be useful to have the text saying 'Migrations added in this PR' so that reviewers have that context. I would also maybe add some text asking the reviewers to do things like check to make sure it won't break blue-green deployments.
| data=$(mysql -h 127.0.0.1 -uedxapp001 -ppassword -e "select app, name from django_migrations ORDER by -id limit $diff;" edxapp;) | ||
| echo "$data" > cleaned_data.txt | ||
| elif [ $number1 -gt $number2 ]; then | ||
| echo "Your PR removed migrations." > cleaned_data.txt |
There was a problem hiding this comment.
I think similar to above, maybe adding some lines about why it might be dangerous to remove migrations would help give this comment more context.
|
app name |
|
Migrations added in this PR |
Just a POC to identify any new migrations coming from edx-platform or any sitepackages.
Upgrade package has 2 migrations.
Added new workflow.
Now checkout current branch.
If there any difference between run this
select name from django_migrations ORDER by -id limit $diffand share as comment on PR.