Adds migration to call handle-artifact-migrations #1174
Conversation
|
I tested without this PR by downgrading to 3.10 and pulp_file 1.5, syncing file content, then upgrading to 3.11 and latest pulp_file, and I could not migraitons or start Pulp. With this PR, that same workflow worked without error. |
|
Attached issue: https://pulp.plan.io/issues/8322 |
| operations = [ | ||
| migrations.RunPython( | ||
| call_handle_artifact_checksums_command, | ||
| reverse_code=call_handle_artifact_checksums_command |
There was a problem hiding this comment.
why is the reverse_code needed? My understanding that it should revert the changes introduced by the migration. It won't revert what it removed I think.
There was a problem hiding this comment.
I'm happy to remove it. I don't think it'll ever be used. I kind of started doing it when I saw @mdellweg doing it, but I don't think it will be used. Should I remove it?
There was a problem hiding this comment.
Let's see what others say, maybe I misunderstand the purpose or something.
There was a problem hiding this comment.
I could go either way. I don't think it'll be useful since a second call to handle-artifact-checksums won't undo the first call but I think calling handle-artifact-checksums is harmless.
There was a problem hiding this comment.
Please keep it. Just the existence of reverse_code marks the migration able to be rolled back (I know this is probably not a requirement for pulpcore, but rollback is a prerequisite to cleanly removing a plugin).
And i don't think that the reverse of a data migration must necessarily produce the original state, but it must ensure that the database is in a consistent state.
Having said that, I'd be fine to use lambda (*): pass as the reverse_code.
There was a problem hiding this comment.
From Django docs, https://docs.djangoproject.com/en/3.1/ref/migration-operations/#runpython
The reverse_code argument is called when unapplying migrations. This callable should undo what is done in the code callable so that the migration is reversible.
I read should undo what is done in the code as bring back to the state it was before the migration code ran.
I'm fine with merging this PR as is.
It's just feels that we are using reverse_code not fully complying with what it is supposed to do. And if it is important for it to be in every migration, and especially for a plugin, maybe we need to figure out some policy or recommended approach for writing migrations.
@mdellweg unfortunately, many migrations are irreversible, that's reality and I'm not sure what to do with that if it's required for a plugin removal.
There was a problem hiding this comment.
Agreed, besides not having reverse in place for existing migrations, they couldn't reproduce the db state that was there before.
To share here for others, at the pulpcore meeting, we decided we'll keep this here since it's not hurting things and @mdellweg will look into what other migrations would need reverse to have a reasonable reversal and see how large the gap is.
|
|
||
|
|
||
| def call_handle_artifact_checksums_command(apps, schema_editor): | ||
| call_command('handle-artifact-checksums') |
There was a problem hiding this comment.
Is it possible to get any failure while running this command? If yes, what will user do?
There was a problem hiding this comment.
It's definitely possible for an error to occur, but I can't think of one specifically. I think this doesn't make the problem worse than it already would be though, even if we did error handling here, pulp will refuse to start just after this and there isn't error handling there either.
There was a problem hiding this comment.
Just so I understand user experience here:
- user runs our installer to upgrade to 3.11
- they run into an error which
handle-artifact-checksumsproduces - they fix some configuration or content or something else
- what's next? they re-run the installer (since the failed migration is not applied)?
There was a problem hiding this comment.
Yes this is what I expect would happen.
pulpcore/app/settings.py
Outdated
| @@ -331,6 +333,8 @@ | |||
| if row[0] > 0: | |||
| if len(sys.argv) >= 2 and sys.argv[1] == "handle-artifact-checksums": | |||
| break | |||
| if len(sys.argv) >= 2 and sys.argv[1] == "migrate": | |||
There was a problem hiding this comment.
You could also
| if len(sys.argv) >= 2 and sys.argv[1] == "migrate": | |
| if len(sys.argv) >= 2 and sys.argv[1] in ["handle-artifact-checksums", "migrate"]: |
There was a problem hiding this comment.
Great idea, I'll change this now.
In 3.11 due to the settings changing, every system will need to call this command, therefore providing it as a data migration is easier for everyone. Users can still modify the `ALLOWED_CONTENT_CHECKSUMS` command as they see fit prior to the 3.11 upgrade and this migration will serve them well too. In order to run the migrations in an environment where the checksum checks in `pulpcore.app.settings` would fail, they have also been adjusted to allow the `pulpcore-manager migrate` command to run. closes #8322
bmbouter
force-pushed
the
auto-call-handle-artifact-checksums-via-migration
branch
from
cb997bf
to
adf4be5
Compare
|
With two lgtm's I'm merging. If anything else needs to be adjusted post-merge please lmk and I will. |
In 3.11 due to the settings changing, every system will need to call
this command, therefore providing it as a data migration is easier for
everyone. Users can still modify the
ALLOWED_CONTENT_CHECKSUMScommand as they see fit prior to the 3.11 upgrade and this migration
will serve them well too.
In order to run the migrations in an environment where the checksum
checks in
pulpcore.app.settingswould fail, they have also beenadjusted to allow the
pulpcore-manager migratecommand to run.closes #8322