-
Notifications
You must be signed in to change notification settings - Fork 79
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Expose the sha256 for source package dsc files #1166
Conversation
We'd like to be able to have a sha256 column for Source Packages like we do for packages. This would save us an extra call to the artifact endpoint. [noissue]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like a reasonable request. Changes look good to me!
Is there any reason why we shouldn't do this @quba42 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daviddavis I do have a question: It looks like the implementation for source packages in this PR is different, from the implementation for other packages. Here you define a property that accesses the sha256
on the associated artifact. While on the BasePackage model, there is an actual sha256 DB column, that is set during sync (but not for source packages): https://github.com/pulp/pulp_deb/blob/main/pulp_deb/app/tasks/synchronizing.py#L1011.
As far as I can tell, this means for Package and InstallerPackage, sha256 is present in the DB both on the content table and the associated artifact, essentially duplicating this data. The implementation in this PR may well be the better approach, but I would like to hear your thoughts on this choice? Might it even be worth to change how Package and InstallerPackage work to be more like the implementation from this PR (not as part of this PR but at some point)?
@property | ||
def sha256(self): | ||
"""Return the sha256 of the dsc file.""" | ||
return self.contentartifact_set.get(relative_path=self.relative_path).artifact.sha256 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this is important, but I tried to look up where contentartifact_set
is defined, and could not find it. Am I just being blind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a method that django defines when you have a many-to-one relationship.
@quba42 I don't know the answer to your questions. I think maybe SourcePackage doesn't have a sha256 db column since unlike Package and InstallerPackage, SourcePackage is a multi-artifact content type so there's not a single sha256 that's directly associated with the source package itself. I think the main factor driving the design of this field though is the sync code and it's been years since I've used or worked with the sync code in Pulp. I would say though that this change doesn't preclude adding a sha256 db column to SourcePackage or removing the sha256 db column from Package/InstallerPackage though. |
It looks like maybe Package has a sha256 field as it's part of its uniqueness constraint. Whereas it doesn't look like SourcePackage has a uniqueness constraint (maybe it should?). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any possible long-term design discussions notwithstanding, I am fine with merging this quality of life improvement as is.
Thanks. I've filed an issue about the missing source package index. |
We'd like to be able to have a sha256 column for Source Packages like we do for packages. This would save us an extra call to the artifact endpoint.
[noissue]