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

Fix sync and upload of the same erratum #852

Merged
merged 1 commit into from
May 3, 2016

Conversation

goosemania
Copy link
Member

Handle pkglist for the same errata in different repositories.
Update errata metadata based on updated field.

closes #858
https://pulp.plan.io/issues/858

@goosemania goosemania force-pushed the issue858 branch 3 times, most recently from afbc7ec to 300a29e Compare April 26, 2016 13:41
@bmbouter bmbouter self-assigned this Apr 27, 2016
@@ -453,6 +455,11 @@ class Errata(UnitMixin, ContentUnit):

SERIALIZER = serializers.Errata

# Erratum fields that can be updated
erratum_fields_update = ('status', 'updated', 'description', 'pushcount', 'references',
Copy link
Member

Choose a reason for hiding this comment

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

Consider renaming this to mutable_erratum_fields which would allow the comment above to be removed. I think the idea that a field is mutable means it can be updated.

@bmbouter
Copy link
Member

@goosemania I'm done w/ my review with probably too many comments. It's really good and it's handling a complicated case. I'm still unclear on one section due to the _pulp_repo_id, but I'll learn more from what you write about that. I'm assigning the PR back to you since I'm done with it. There are a lot of changes so we should review again after another revision. It's really good work!

:return: True if the other erratum is newer than the existing one
:rtype: bool
"""
err_msg = _('Fail to update the %(which)s erratum %(id)s: '
Copy link
Member

Choose a reason for hiding this comment

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

nice internationalization 🐈 🎏 🐠

@bmbouter bmbouter assigned bmbouter and goosemania and unassigned bmbouter May 2, 2016
existing_unit.merge_errata(new_unit)
unit = existing_unit

# No need to catch NotUniqueError here. We save either the new erratum or the merged one.
Copy link
Member

Choose a reason for hiding this comment

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

This comment can probably be removed.

@bmbouter
Copy link
Member

bmbouter commented May 2, 2016

@goosemania This is really good. I left a few small comments, but otherwise I think it looks good to merge. I'm adding the LGTM flag.

There is just 1 area that probably needs a little bit of testing work. The redmine story talks about pulp-smash tests which will make use of this. Instead of doing them along with this story as the issue suggests I recommend the following:

  1. create an issue at https://github.com/PulpQE/pulp-smash/issues which will track the writing of a test for the updated errata case
  2. Have the original and "updated" metadata added to the pulp-fixtures repo where a test_repo can be generated which can simulate an errata unit being updated both via sync and upload.

Handle pkglist for the same errata in different repositories.
Update errata metadata based on `updated` field.

closes pulp#858
https://pulp.plan.io/issues/858
@goosemania goosemania merged commit a8b0d87 into pulp:2.8-dev May 3, 2016
@goosemania goosemania deleted the issue858 branch May 12, 2016 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants