Add model fields for Package type #1105
Conversation
dralley
force-pushed
the
3199-models
branch
16 times, most recently
from
aa5a64b
to
4dc2beb
Compare
dralley
force-pushed
the
3199-models
branch
5 times, most recently
from
155bf41
to
73f07cc
Compare
| # version (str): version | ||
| # release (str): release | ||
| # pre (bool): preinstall | ||
| requires = models.TextField(default='[]', blank=True) |
There was a problem hiding this comment.
How do 'rich dependencies' fit in here? I couldn't find any information with respect to createrepo_c + 'rich' or 'boolean' dependencies.
Is it something we don't really have to be concerned with at the modelling level?
There was a problem hiding this comment.
Yeah, I believe we should not be concerned about that for the modeling part.
Any dependency solving should be handled by an external library and it's its business to figure out all the relationships and types of dependencies the package has by repository and package metadata.
And for createrepo_c I'd guess that any rich dependency would be just a string which is not parsed.
| ) | ||
|
|
||
|
|
||
| class SrpmContent(RpmContent): |
There was a problem hiding this comment.
Should this be a separate content type, or an is_srpm=True boolean flag on the RPM model?
There was a problem hiding this comment.
I'm personally +1 to have a separate type. It will be a separate table in DB, right? I think it will make all the type related operations easier.
There was a problem hiding this comment.
Regardless of how it's modeled. it would be great if when filtering packages that you have 1 endpoint that has both rpms and srpms with a flag like is_srpm=True. With that in mind modeling it as the same may be the easiest. I'm ok with either approach.
There was a problem hiding this comment.
I have a general question.
Do we really want to have all those fields on the model?
Are we going to use fields like rpm_buildhost or time_build and other similar ones?
I agree not to create/name fields which will be different from what createrepo_c has but I'm not sure we need all the attributes from createrepo_c objects until we have a use case for them.
From a migration point of view, it's ok until we have lazy because we can take this data from the rpm itself. Thoughts?
| recommends = models.TextField(default='[]', blank=True) | ||
| supplements = models.TextField(default='[]', blank=True) | ||
|
|
||
| location_base = models.TextField(blank=True) |
There was a problem hiding this comment.
Can be null, not just empty string. FWIW, I'm testing on the packages from EPEL repo.
There was a problem hiding this comment.
Same for at least url and description
There was a problem hiding this comment.
Django heavily recommends that you not use null=True with TextField or CharField because it just creates extra "empty" states.
We can do it, but it would be preferable to just store None as an empty string.
|
|
||
| class Meta: | ||
| unique_together = ( | ||
| 'name', 'epoch', 'version', 'release', 'arch', 'checksum_type', 'pkgId' |
There was a problem hiding this comment.
Can someone remind me, why we use checksum_type and checksum(pkgId) as a part of the unique index?
Why can't we leave just checksum_type? The only reason to support two packages with the same nevra and checksum_type but different checksum(pkgId) is if one package is wrong and another one is a correct one which was fixed/updated, thus checksum(pkgId) is different. What do you all think? Is it a valid use case?
Why can't we just leave checksum(pkgId) in the unique index? It will cover the use case ^
There was a problem hiding this comment.
The checksum value gives an easy way to know if the same package has been signed by different gpg keys (or no key at all). That's the primary reason checksum is included in the unit key. It could be better to use information about the signature directly in the unit key, but that's a lot harder to access since it isn't included in any of the XML metadata. You need to inspect the RPM file directly to get at it.
Assuming you keep the checksum value in the unit key, the checksum type isn't very helpful for establishing uniqueness. But it's such a fundamental part of the checksum value, that it would be weird to have a value without knowing the type. So that's why both have been included in the unit key.
pulp_rpm/app/models.py
Outdated
|
|
||
| (same as the "rpm" content type) | ||
| """ | ||
| TYPE = 'rpm' |
Good question. Pulp 2 does model them, but does it actually use them? https://github.com/pulp/pulp_rpm/blob/pulp-rpm-2.14.3-1/plugins/pulp_rpm/plugins/db/models.py#L751 |
dralley
force-pushed
the
3199-models
branch
2 times, most recently
from
64d66ba
to
416cde3
Compare
|
Added |
| location_base = models.TextField(blank=True) | ||
| location_href = models.TextField(blank=True) | ||
|
|
||
| rpm_buildhost = models.TextField(blank=True) |
There was a problem hiding this comment.
why are these particular fields prefixed with rpm_?
There was a problem hiding this comment.
because that's what createrepo_c does, and we're matching what they do precisely
pulp_rpm/app/models.py
Outdated
|
|
||
| (same as the "rpm" content type) | ||
| """ | ||
| TYPE = 'source-rpm' |
There was a problem hiding this comment.
I suggest to name it "srpm", this name will appear in the API or in the filters,
closes #3199
https://pulp.plan.io/issues/3199