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

Save modifications to the erratum pkglist before adding new collections. #869

Merged
merged 1 commit into from
May 18, 2016

Conversation

goosemania
Copy link
Member

It is needed because mongoengine does not allow to modify items in
the list and to add a new ones to the list at the same time.
And sometimes we are in this situation during the update of the erratum.

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

@bmbouter bmbouter self-assigned this May 17, 2016
# collections in the pkglist in the database before new collections will be added to
# the pkglist, because mongoengine does not allow to modify existing items in the list
# and add new items to the list at the same time.
self.save()
Copy link
Member

@bmbouter bmbouter May 17, 2016

Choose a reason for hiding this comment

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

Because of this change, I propose the function name change from merge_pkglists to merge_pkglists_and_save. This will more accurately capture the new side-effect this introduces.

Also does it make sense to have an additional save() after L#777 so that the final state is also saved? I think it would be good but you would know better than I about that. Since the function is providing a save() functionality we should have it save() the final state also and not just the partial state. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was concerned about this partial state, but even adding another save() at the end of this method is still the partial update of the erratum, but it can be a final state for merging pkglists.

Copy link
Member

Choose a reason for hiding this comment

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

Adding the additional save() would be good for the reason you mention in your comment.

@bmbouter
Copy link
Member

I'm adding a LGTM because I don't need to re-review. Please do making the additional save() call and updating the name of the method per the comments before merging.

Good fix! 🌴 😄 🐈

@bmbouter bmbouter added the LGTM label May 17, 2016
@bmbouter bmbouter assigned goosemania and unassigned bmbouter May 17, 2016
It is needed because mongoengine does not allow to modify items in
the list and to add a new ones to the list at the same time.
And sometimes we are in this situation during the update of the erratum.

closes pulp#1910
https://pulp.plan.io/issues/1910
@goosemania
Copy link
Member Author

ok test

@goosemania goosemania merged commit cab29d0 into pulp:2.8-dev May 18, 2016
@goosemania goosemania deleted the issue1910 branch May 18, 2016 11:38
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.

2 participants