Remove django fernet #447
Remove django fernet #447
Conversation
| @@ -170,8 +169,6 @@ class Migration(migrations.Migration): | |||
| ('modified', model_utils.fields.AutoLastModifiedField(default=django.utils.timezone.now, editable=False, verbose_name='modified')), | |||
| ('org', models.CharField(help_text='This value must match the value of organization in studio/edx-platform.', max_length=50, verbose_name='Organization')), | |||
| ('provider', models.CharField(choices=[('Custom', 'Custom'), ('3PlayMedia', '3PlayMedia'), ('Cielo24', 'Cielo24')], max_length=50, verbose_name='Transcript provider')), | |||
| ('api_key', fernet_fields.fields.EncryptedTextField(max_length=255, verbose_name='API key')), | |||
There was a problem hiding this comment.
This model already deleted in last migration. So removing these fields will not effect stage or prod since these are already executed there.
https://github.com/openedx/edx-val/blob/master/edxval/migrations/0003_delete_transcriptcredentials.py
There was a problem hiding this comment.
This needs more thought. It's not just our stage/prod, but every Open edX deployment across the globe that needs to be accounted for.
The simple way to handle this is squashing migrations: https://openedx.atlassian.net/wiki/spaces/AC/pages/23003228/Everything+About+Database+Migrations#EverythingAboutDatabaseMigrations-SquashingMigrations. However, the old outdated migrations would normally not be deleted until after the next Open edX named release.
Is that too late? If so, this needs some additional thinking. For example, maybe the migration that removes the fields would need to be conditional on whether they actually exist? But editing old migrations is always scary.
There was a problem hiding this comment.
These models/migrations were delete around May 27, 2020.
There was a problem hiding this comment.
- The safe way to do this is to squash migrations. The problem is that it wouldn't matter how old the migrations are, you'd need to wait until everyone has run the squashed migration before deleting the old, which means waiting through a named release cycle.
- The riskier way of doing this is to edit the old migrations.
- If you choose to go this path, I'd make this all more clear by removing the entire CreateModel for
TranscriptCredentialsin this migration, and the entire delete model for0003. You could explain everything in the PR comment and description. - The problem would be if 0001 and 0003 were added as parts of different named releases. In that case, someone could have a DB that has run 0001 and not 0003, which is what squashing takes into account.
- An alternative is to remove the CreateModel from 0001, but make 0003 remove the model only if needed. I'm not sure exactly how that would be coded, but that should take care of this weird case.
- Or, if 0001 and 0003 were both added in the same single named release, then it may be safe to edit both at the same time.
- If you choose to go this path, I'd make this all more clear by removing the entire CreateModel for
There was a problem hiding this comment.
Ugh. I realize that in requesting that the whole model be deleted to add clarity, I inadvertently made it all more complex. I think your solution is probably fine as-is, but I just want a little more time to think about it. Thank you.
There was a problem hiding this comment.
Sorry it took me some time to grasp this more completely. I think this covers all the cases I was concerned with as-is. Thank you.
There was a problem hiding this comment.
Thanks @robrap for detailed review. As always, it contains incredibly valuable guidelines.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #447 +/- ##
=======================================
Coverage 94.40% 94.40%
=======================================
Files 28 28
Lines 3001 3001
Branches 168 168
=======================================
Hits 2833 2833
Misses 150 150
Partials 18 18
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
| @@ -170,8 +169,6 @@ class Migration(migrations.Migration): | |||
| ('modified', model_utils.fields.AutoLastModifiedField(default=django.utils.timezone.now, editable=False, verbose_name='modified')), | |||
| ('org', models.CharField(help_text='This value must match the value of organization in studio/edx-platform.', max_length=50, verbose_name='Organization')), | |||
| ('provider', models.CharField(choices=[('Custom', 'Custom'), ('3PlayMedia', '3PlayMedia'), ('Cielo24', 'Cielo24')], max_length=50, verbose_name='Transcript provider')), | |||
| ('api_key', fernet_fields.fields.EncryptedTextField(max_length=255, verbose_name='API key')), | |||
There was a problem hiding this comment.
Sorry it took me some time to grasp this more completely. I think this covers all the cases I was concerned with as-is. Thank you.
Trying to remove
djfernetfrom this repo. Itsun-usedimport exists in migration.Model is already removed in following prs.
#229
#232
These two PRs removed actual code/models but some thing still exists in migration.
Will remove
djfernetin next release.