Add labels to domains #6254
Add labels to domains #6254
Conversation
gerrod3
force-pushed
the
domain-labels
branch
from
cecae09 to
257bcbb
Compare
gerrod3
force-pushed
the
domain-labels
branch
from
1b3e0e4 to
46cee20
Compare
gerrod3
force-pushed
the
domain-labels
branch
from
46cee20 to
671334d
Compare
There was a problem hiding this comment.
Can you elaborate on changing this old migration?
There was a problem hiding this comment.
So as you can see the CI is failing on the migrations because I made the grave mistake of calling outside model code (get_domain_pk) from inside a migration (any migration adding a pulp_domain relation). On a new installation this causes the old migrations to fail now since I've added a new field to the Domain model which means the SQL the outside code generates will be too far ahead of the database at the time of the migration. So I had to change this migration and I'm going to have to change get_domain_pk to only pull in the pk.
There was a problem hiding this comment.
So I see the add_domain_relations migration failing. I think we should fix that one instead.
gerrod3
force-pushed
the
domain-labels
branch
from
671334d to
23c23ed
Compare
| @@ -58,4 +66,5 @@ class Migration(migrations.Migration): | |||
| bases=(django_lifecycle.mixins.LifecycleModelMixin, models.Model, pulpcore.app.models.access_policy.AutoAddObjPermsMixin), | |||
| ), | |||
| migrations.RunSQL(DEFAULT_DELETE_TRIGGER, reverse_sql=REMOVE_DEFAULT_DELETE_TRIGGER), | |||
| migrations.RunPython(code=create_default_domain, reverse_code=migrations.RunPython.noop), | |||
There was a problem hiding this comment.
As a rule of thumb we established that runpython is good to migrate existing data to a new format. But for creating new data, you should really rely on the post-migrate hook.
There was a problem hiding this comment.
Not possible. We need the default domain for the next migration to work. Without this we can't start up a clean Pulp instance.
pulpcore/app/util.py
Outdated
| @@ -580,7 +580,8 @@ def get_default_domain(): | |||
| except AttributeError: | |||
| return None | |||
| try: | |||
| default_domain = Domain.objects.get(name="default") | |||
| queryset = Domain.objects.only("pk") if pk_only else Domain.objects | |||
| default_domain = queryset.get(name="default") | |||
There was a problem hiding this comment.
This line will cache whatever we received as the domain. Even if it was only the shallow "pk only" version.
can we, instead leave this function alone and add a fallback implementation to get_domain_pk. With a big banner that it will be used in migrations...
gerrod3
force-pushed
the
domain-labels
branch
2 times, most recently
from
a8c46da to
40d3569
Compare
pulpcore/app/util.py
Outdated
| """ | ||
| THIS CAN/WILL BE RAN IN MIGRATIONS. DO NOT USE ORM FOR IT MIGHT GENERATE INVALID SQL. | ||
| """ |
There was a problem hiding this comment.
Thank you! This will help us not break it in the future.
There was a problem hiding this comment.
Can you add that we use it in plugin migrations so there is no way to move this implementation and we must not change it's semantics ever.
Feels like BaseDistribution reloaded.
pulpcore/app/util.py
Outdated
| """ | ||
| THIS CAN/WILL BE RAN IN MIGRATIONS. DO NOT USE ORM FOR IT MIGHT GENERATE INVALID SQL. | ||
| """ |
There was a problem hiding this comment.
Can you add that we use it in plugin migrations so there is no way to move this implementation and we must not change it's semantics ever.
Feels like BaseDistribution reloaded.
mdellweg
dismissed
their stale review
The situation around migrations including ones of dependent plugins does not allow to solve this "properly".
Thank you for the detailed explanation.
Also modify get_domain_pk to use raw SQL to avoid problems with running inside migrations
gerrod3
force-pushed
the
domain-labels
branch
from
40d3569 to
2dcb98d
Compare
|
Thank you! |
fixes: #6236
Side note: I used Cursor & its agent feature to make this commit. With just some minor edits, it correctly added everything on its own. It even knew to run the migration command after adding the field (of course I had to manually run it since it didn't know about our oci-env). Maybe we should take a look at our issue backlog and note the simple/medium issues that we can throw to the AI (I'll take requests 😄).