Skip to content
This repository has been archived by the owner on Jan 9, 2023. It is now read-only.

Add the pulp2_subid field to the Pulp2Content model #295

Merged
merged 1 commit into from Feb 4, 2021

Conversation

quba42
Copy link
Contributor

@quba42 quba42 commented Jan 21, 2021

fixes #8137
https://pulp.plan.io/issues/8137

The intended use case is to provide one to many mapping from Pulp 2 to
Pulp 3 content. This is achieved by creating multiple Pulp2Content
entries that all share a single pulp2_id, and differ only via the
pulp2_subid.

This is needed for structured Debian migrations:
https://pulp.plan.io/issues/7865

The extra Pulp2Content subrecords are added by the relevant
pre_migrate_content_detail method. The reason is that it is only known
what subrecords are needed once the content is actually inspected by
this method.

@quba42
Copy link
Contributor Author

quba42 commented Jan 21, 2021

Note that the approach was taken from here: #292

The new field is needed for: #273

@quba42
Copy link
Contributor Author

quba42 commented Jan 21, 2021

Should I rename pulp_2to3_migration/app/migrations/0021_auto_20210121_1112.py to something more meaningful?

@quba42
Copy link
Contributor Author

quba42 commented Jan 21, 2021

Not sure why this would cause test failures for RPM.

@ipanova
Copy link
Member

ipanova commented Jan 21, 2021

Should I rename pulp_2to3_migration/app/migrations/0021_auto_20210121_1112.py to something more meaningful?

yes please.

@goosemania
Copy link
Member

goosemania commented Jan 22, 2021

Not sure why this would cause test failures for RPM.

Interesting, I wonder if it happens because every null is unique and is a part of uniqueness constraint.
Try to remove null=True and use an empty string as a default.

@dralley , @ipanova , any thoughts on this?

@quba42
Copy link
Contributor Author

quba42 commented Jan 25, 2021

Interesting, I wonder if it happens because every null is unique and is a part of uniqueness constraint.
Try to remove null=True and use an empty string as a default.

I found this in the Django docs:

Avoid using null on string-based fields such as CharField and TextField because empty string values will always be stored as empty strings, not as NULL. If a string-based field has null=True, that means it has two possible values for “no data”: NULL, and the empty string. In most cases, it’s redundant to have two possible values for “no data;” the Django convention is to use the empty string, not NULL.

@goosemania
Copy link
Member

@quba42 , your changes look good, thanks!
The CI is temperamental and we are looking at why it is so, it happens to many PRs, not only yours.

fixes #8137
https://pulp.plan.io/issues/8137

The intended use case is to provide one to many mapping from Pulp 2 to
Pulp 3 content. This is achieved by creating multiple Pulp2Content
entries that all share a single pulp2_id, and differ only via the
pulp2_subid.

This is needed for structured Debian migrations:
https://pulp.plan.io/issues/7865

The extra Pulp2Content subrecords are added by the relevant
pre_migrate_content_detail method. The reason is that it is only known
what subrecords are needed once the content is actually inspected by
this method.
@@ -33,9 +33,10 @@ class Pulp2Content(BaseModel):
downloaded = models.BooleanField(default=False)
pulp3_content = models.ForeignKey(Content, on_delete=models.SET_NULL, null=True)
pulp2_repo = models.ForeignKey(Pulp2Repository, on_delete=models.SET_NULL, null=True)
pulp2_subid = models.CharField(max_length=255, blank=True)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Should I send something on the mailing list that blank=True is preferable to null=True for string fields? I am pretty sure there are a lot of null=True string fields around the various Pulp repositories. How concerned should we be about this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's always good to have such conversations on pulp-dev. I thought we had similar in the past but if we did it was looong ago. +1 to write to the mailing list and see why we didn't go that path in some cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@goosemania goosemania merged commit cfc8ec1 into pulp:master Feb 4, 2021
@quba42 quba42 deleted the one_to_many_migrating branch February 4, 2021 13:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants