Extend finalize_new_version to resolve advisory duplicates #1513
Conversation
|
Hello @goosemania! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2019-11-15 21:40:16 UTC |
goosemania
force-pushed
the
story4295
branch
from
2b58867
to
a613c17
Compare
goosemania
changed the title
goosemania
force-pushed
the
story4295
branch
from
a613c17
to
d754467
Compare
| @@ -528,8 +580,7 @@ class UpdateCollection(Model): | |||
| shortname = models.TextField() | |||
| module = models.TextField(default='') | |||
|
|
|||
| update_record = models.ForeignKey(UpdateRecord, related_name="collections", | |||
| on_delete=models.CASCADE) | |||
| update_record = models.ManyToManyField(UpdateRecord, related_name="collections") | |||
There was a problem hiding this comment.
This is needed not to duplicate collections when we merge advisories.
Migrations are squashed due to this change.
goosemania
force-pushed
the
story4295
branch
from
d754467
to
e926273
Compare
goosemania
force-pushed
the
story4295
branch
3 times, most recently
from
4fceace
to
19dd96a
Compare
goosemania
force-pushed
the
story4295
branch
2 times, most recently
from
4cb4b53
to
1dbabf3
Compare
goosemania
force-pushed
the
story4295
branch
from
5121310
to
c4790ad
Compare
pulp_rpm/app/advisory.py
Outdated
| previous_advisory = previous_advisories.get( | ||
| rpm_updaterecord__id=advisory_id).rpm_updaterecord | ||
| added_advisory = version.content.get( | ||
| rpm_updaterecord__id=advisory_id, pk__in=added_advisory_pks).rpm_updaterecord |
There was a problem hiding this comment.
If you convert previous_advisories to UpdateRecords:
previous_advisories = UpdateRecord.filter(pk__in=previous_version.content.filter(pulp_type='rpm.advisory'))You could simplify these lines:
previous_advisory = previous_advisories.get(id=advisory_id)
added_advisory = version.content.get(id=advisory_id, pk__in=added_advisory_pks)There was a problem hiding this comment.
Also, does this not work?
added_advisory = version.content.get(id=advisory_id, pk__in=added_advisories)There was a problem hiding this comment.
@daviddavis, I'll try.
Well, maybe it's the right thing to do everything with the same approach, so I'll probably convert to use UpdateRecord, similar to how you use Modulemd. I just want travis to pass, and then I cam refactor. Testing of correct merging takes some time as well.
There was a problem hiding this comment.
Also, does this not work?
added_advisory = version.content.get(id=advisory_id, pk__in=added_advisories)
This doesn't work because content is the master model, so it will be rpm_updaterecord__id=advisory_id but the main problem is in added_advisories.
I get django.db.utils.ProgrammingError: subquery has too many columns (either by using version.content or UpdateRecord). It's all because added_advisories is a result of qs.difference. So I'm back to collecting pks.
goosemania
force-pushed
the
story4295
branch
from
7dab419
to
1390319
Compare
pulp_rpm/app/advisory.py
Outdated
| if previous_version: | ||
| previous_advisories = previous_version.content.filter(pulp_type='rpm.advisory') | ||
| previous_advisory_ids = set(previous_advisories.values_list('rpm_updaterecord__id', | ||
| flat=True)) |
There was a problem hiding this comment.
If you convert previous_advisories to UpdateRecords (see my other comment), you can simplify this:
previous_advisory_ids = set(previous_advisories.values_list('id', flat=True))
goosemania
force-pushed
the
story4295
branch
from
1390319
to
c230f49
Compare
Also move createrepo_c object generation for advisory to corresponding models. Add ability to generate createrpeo_c object with additional collecitons. closes #4295 https://pulp.plan.io/issues/4295
goosemania
force-pushed
the
story4295
branch
from
c230f49
to
1d507db
Compare
Also move createrepo_c object generation for advisory to corresponding
models. Add ability to generate createrpeo_c object with additional
collecitons.
closes #4295
https://pulp.plan.io/issues/4295