converted to use mongoengine #56
Conversation
| 'checksum_type': p.metadata['_checksum_type'], | ||
| fields = ('version', '_filename', '_checksum', '_checksum_type', 'name', '_storage_path') | ||
| unit_querysets = repo_controller.get_unit_model_querysets(repo_id, models.Package) | ||
| unit_querysets = (q.only(*fields) for q in unit_querysets) |
|
Does there need to be any migrations for this? The data layer should stay the same, but what about dropping old indexes since new ones will be created that may be slightly different. Typically the other conversions have had some index migration work and docs made for them. I'm not sure in this case. |
|
Once merged, the Python models should be added to the validation_script. Perhaps the validation script should be refactored to test with all models as discovered through entry points? That would handle this problem more generally across the plugins without having to update the validation script each time. What do you think? |
|
I'm done w/ the review. There are some comments to handle, but I don't need to re-review as long as you're satisfied. I'm not sure why travis and jenkins aren't happy, but my local tests ran fine so I think it's ok to merge red in this case. |
|
There were only two indexes present before the conversion, and they are both created identically by mongoengine. There is an additional index created by mongoengine, which it does create successfully during upgrade. So I think we're ok without a migration. |
|
I think this is ready to merge, but I'd like @rbarlow to at least take a glance at it first. |
|
ok test |
| """ | ||
| Injects download requests into the call to the parent class's __init__ | ||
| """ | ||
| kwargs['downloads'] = self.generate_download_requests() |
There was a problem hiding this comment.
Since downloads is a required param, it should be listed explicitly in the method parameters and have the standard docblock entries.
There was a problem hiding this comment.
downloads is not a required param. In fact if a caller did pass it in, we overwrite that value here.
There was a problem hiding this comment.
On Thu, Jan 14, 2016 at 06:23:54AM -0800, Michael Hrivnak wrote:
kwargs['downloads'] = self.generate_download_requests()
downloadsis not a required param. In fact if a caller did pass it in, we overwrite that value here.
Ugh, I really hope I only wrote that because I hadn't had enough coffee
yet. Speaking of which, is it… coffee time?
Randy Barlow
irc: bowlofeggs
|
I think you should make that downloads parameter be explicit before merging, but other than that LGTM. |
fixes #877 fixes #1465 https://pulp.plan.io/issues/877 https://pulp.plan.io/issues/1465 1465 was fixed by accident in the course of doing the conversion
|
Scratch builds passed on rhel6, rhel7, and f23. |
converted to use mongoengine
fixes #877
fixes #1465
https://pulp.plan.io/issues/877
https://pulp.plan.io/issues/1465
1465 was fixed by accident in the course of doing the conversion