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

Fixing dupe ISO units after file update #1058

Merged
merged 1 commit into from
Jul 3, 2017
Merged

Conversation

daviddavis
Copy link
Contributor

Fixing a bug whereby an ISO or file gets updated and then reimported into
pulp by sync or upload. We're now disassociating the original content
unit and replacing it with the new one. Matching on the name of the
unit.

fixes #2773
https://pulp.plan.io/issues/2773

@mention-bot
Copy link

@daviddavis, thanks for your PR! By analyzing the history of the files in this pull request, we identified @jortel, @jeremycline and @ipanova to be potential reviewers.

@daviddavis daviddavis changed the base branch from master to 2.13-dev June 23, 2017 16:37
Fixing a bug whereby a ISO or file gets updated and then reimported into
pulp by sync or upload. We're now disassociating the original content
unit and replacing it with the new one. Matching on the name of the
unit.

fixes pulp#2773
https://pulp.plan.io/issues/2773
@goosemania
Copy link
Member

@daviddavis , it looks like the similar issue was resolved for RPMs sync/upload, there is a purge.remove_unit_duplicate_nevra for that and it is written in quite general way, not general enough to use for ISO right away but maybe it can become less unit specific. What do you think? I am not entirely sure if it's a good idea, just a thought.
Another observation is that RPM importer purges units from repo before importing new ones into Pulp and you do it after. I am not sure about the reason for that but if it doesn't matter I suggest to do it in a similar way for consistency.

Copy link
Member

@goosemania goosemania left a comment

Choose a reason for hiding this comment

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

I left a comment to discuss general approach to solve this issue, could you take a look

@daviddavis
Copy link
Contributor Author

daviddavis commented Jun 28, 2017

@goosemania thanks for the feedback. I can take a look at combining the iso code with purge.remove_unit_duplicate_nevra. Where does code shared between iso and yum importers typically live?

I'm not sure why the rpm importer purges units before importing the new ones, but it seems like a bad idea. If the import fails, then the units would automatically be removed from the repo without the new unit being added to Pulp/the repo. That seems suboptimal.

@daviddavis
Copy link
Contributor Author

daviddavis commented Jun 30, 2017

@goosemania pushed the changes in a new commit. There may be a better way to generalize remove_unit_duplicate_nevra..? Also, I am not sure what to do about the _associate_unit method since it uses caching instead of just removing the unit. I suppose I could dump that though and use remove_unit_duplicate_values as is.

@daviddavis
Copy link
Contributor Author

I'm a bit torn. Not sure if the generalization here is an improvement but maybe there's a better way?

@goosemania
Copy link
Member

@daviddavis , yeah, I am not sure either.
Maybe just passing attribute names to exclude from unit_key will make it better? In this case some changes will be needed for yum distributor though.
But now I think you convinced me to stay with your initial approach.

@daviddavis
Copy link
Contributor Author

@goosemania reverted to the initial commit. I think it's the simplest solution.

@goosemania
Copy link
Member

@daviddavis , thanks! 🍻

@daviddavis daviddavis merged commit cda2310 into pulp:2.13-dev Jul 3, 2017
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

3 participants