Skip to content
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

Add modelling for "Errata" content #1109

Merged
merged 1 commit into from Aug 2, 2018
Merged

Conversation

dralley
Copy link
Contributor

@dralley dralley commented May 12, 2018


collections = models.TextField(blank=True) # formerly "pkglist"
references = models.TextField(blank=True)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With UpdateCollections and UpdateReferences, much like with "depends" we can either go the route of JSON representation, or make actual models with a foreign key that provides a related_name of "collections".

Do we have a preference? Is there value in modelling it vs. shoving it into JSON?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pulp 2 did model "pkglist" / collections separately

Copy link
Member

Choose a reason for hiding this comment

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

I suggest to go with the simplest solution at this point aka JSON. We can move to models later if we need to.
I hope we won't take the approach similar to pulp 2 for errata :)

Copy link
Member

Choose a reason for hiding this comment

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

+1 to JSON for simplicity. Are we using the custom json field or just storing it as a StringField and serializing/deserailizing when we read/write?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TextField for now... messing with JSONField directly is fraught with enough pitfalls generally speaking that until further notice I wouldn't want to rely on it. It works fine for reading (how we use it with Tasks) but writing is really hard to make work properly

Copy link
Member

Choose a reason for hiding this comment

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

Great. +1 to using TextField for now.

@dralley dralley force-pushed the 3201-errata-model branch 2 times, most recently from 8e91c06 to 5f2aa44 Compare May 15, 2018 18:58
@dralley dralley requested a review from goosemania May 15, 2018 19:01
@dralley dralley force-pushed the 3201-errata-model branch 2 times, most recently from 8c1bfd1 to 8786af7 Compare May 17, 2018 21:26
@dralley
Copy link
Contributor Author

dralley commented May 17, 2018

@goosemania Updated. I will squash the commits before we merge.

@dralley dralley force-pushed the 3201-errata-model branch 2 times, most recently from 28e75d0 to ab80e6b Compare May 18, 2018 19:34
@dralley
Copy link
Contributor Author

dralley commented May 19, 2018

The "id" field on the UpdateRecord model conflicts with the "id" field on the Content model, we need to rename one of them

@bmbouter
Copy link
Member

@dralley I think we need to change the "id" on the "Content model itself to pulp_id. This is the second content type I've heard that can't use the expected field name because Pulp is already asserting it.

@dralley
Copy link
Contributor Author

dralley commented May 23, 2018

@bmbouter Should we take it to #pulp-dev then?

@bmbouter
Copy link
Member

@dralley dralley force-pushed the 3201-errata-model branch 2 times, most recently from 97329d7 to 7fb01c4 Compare May 23, 2018 19:13
@dralley dralley force-pushed the 3201-errata-model branch 2 times, most recently from 5905e11 to 8fee4d7 Compare June 6, 2018 19:44
@daviddavis daviddavis changed the base branch from 3.0-dev to master July 2, 2018 20:44
@daviddavis daviddavis changed the base branch from master to 3.0-dev July 2, 2018 20:45
@daviddavis daviddavis changed the base branch from 3.0-dev to master July 2, 2018 20:47
# Bookeeping
uniqueness_hash = models.TextField(unique=True)

@property
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have not tested this, it probably doesn't work. It's just a conceptual thing

@@ -200,6 +203,184 @@ class SrpmContent(RpmContent):
TYPE = 'srpm'


class UpdateRecord(Content):
"""
The "UpdateRecord" content type. Formerly "Errata".
Copy link
Contributor

Choose a reason for hiding this comment

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

s/Errata/Erratum/ ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, can you maybe clarify this comment then by saying something like 'Formerly "Errata" in Pulp 2'?

@daviddavis
Copy link
Contributor

For some models in pulp_rpm, we're using Content in the name (RpmContent, SrpmContent) but these records lack the Content suffix. What about renaming RpmContent and SrpmContent to Rpm and Srpm to be consistent?

@dralley
Copy link
Contributor Author

dralley commented Jul 5, 2018

Is that a problem? In the Python plugin, the model is named PythonPackageContent but we don't use the content suffix either.

@daviddavis
Copy link
Contributor

daviddavis commented Jul 5, 2018

I think it's inconsistent. It should either be UpdateRecordContent, RpmContent, etc or UpdateRecord, Rpm, etc.

@dralley
Copy link
Contributor Author

dralley commented Jul 5, 2018

So, I think this is an interesting question. One point: ATM the Errata-related models don't subclass Content because I thought that it might be inappropriate, given that unlike packages, errata do not have have a 1-to-1 corresponding artifact. I think they're all stored together in updateinfo.xml, afaik.

@bmbouter Do you know if that is correct and/or have input? I should have brought this up as a discussion topic at some point.

@daviddavis
Copy link
Contributor

Doesn't UpdateRecord subclass Content though?

@daviddavis
Copy link
Contributor

Also, I'm not arguing for or against subclassing Content. Just pointing out that the class names are inconsistent.

@daviddavis
Copy link
Contributor

So, I think this is an interesting question. One point: ATM the Errata-related models don't subclass Content because I thought that it might be inappropriate, given that unlike packages, errata do not have have a 1-to-1 corresponding artifact.

I think the way you have it is correct. The litmus test is probably whether a content unit can be added to a repository via RepositoryContent and have endpoints under places like /pulp/api/v3/content/.... Not whether the content unit has artifacts. So UpdateRecord should be the only erratum-related class to extend Content.

Another example of an artifact-less content unit is package group.

@dralley dralley merged commit bb1590b into pulp:master Aug 2, 2018
@dralley dralley deleted the 3201-errata-model branch August 2, 2018 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants