Fix remove_missing option and unnecessary downloads for forge importer #255
Conversation
|
@goosemania, thanks for your PR! By analyzing the history of the files in this pull request, we identified @mhrivnak, @jortel and @bmbouter to be potential reviewers. |
| @@ -120,7 +132,7 @@ def run(self, **kwargs): | |||
|
|
|||
| # Remove the importer keys from kwargs so they don't get added to the repo config | |||
| for key in importer_config: | |||
| kwargs.pop(key, None) | |||
| kwargs.pop(key.replace('_', '-'), None) | |||
| @@ -304,7 +305,7 @@ def _remove_missing(self, existing_module_ids_by_key, remote_unit_keys): | |||
| keys_to_remove = list(set(existing_module_ids_by_key.keys()) - set(remote_unit_keys)) | |||
| doomed_ids = [existing_module_ids_by_key[key] for key in keys_to_remove] | |||
| doomed_module_iterator = Module.objects.in_bulk(doomed_ids).itervalues() | |||
| repo_controller.disassociate_units(self.repo, doomed_module_iterator) | |||
| repo_controller.disassociate_units(self.repo.repo_obj, doomed_module_iterator) | |||
There was a problem hiding this comment.
Was this part of the plugin completely broken? It seems that way to me.
There was a problem hiding this comment.
@dkliban , yes, it was.
| """ | ||
| return list(set(metadata_unit_keys) - set(existing_unit_keys)) | ||
| model = plugin_api.get_unit_model_by_id(constants.TYPE_PUPPET_MODULE) |
There was a problem hiding this comment.
i guess there is an issue somewhere around here because it is not able to correctly detect what to download.
create a repo and set query=stdlib. It will not download any modules, meanwhile on master branch it will fetch and import into the repo 19 modules. Alos this outpit https://forge.puppet.com/modules.json?q=stdlib proves that there are modules on puppetforge meeting the query. There was performed an orphan removal before the repo creation.
There was a problem hiding this comment.
@ipanova, thanks! Fixed.
|
This was my testing approach pulp/pulp-smash#578 |
Fix remove_missing option for the forge and directory importers. Do not re-download units if they are in DB and on a disk for the forge importer. closes #2606 https://pulp.plan.io/issues/2606
| for unit in units_controller.find_units(unit_generator): | ||
| file_exists = unit._storage_path is not None and os.path.isfile(unit._storage_path) | ||
| if file_exists: | ||
| if unit.unit_key_as_named_tuple not in existing: |
There was a problem hiding this comment.
it is purely matter of personal preference but i would move line 341 to right after condition on line 338 happens, it is more readable imho
|
@goosemania, fyi, i've tested the sync with directory as well with remove-missing option. |
Fix remove_missing option for the forge and directory importers.
Do not re-download units if they are in DB and on a disk for the forge importer.
closes #2606
https://pulp.plan.io/issues/2606