Tests get imageRef from nova #1835
Conversation
instead of IMAGE_REF env.
instead of configured imageRef
|
@glyph review please? Meanwhile, let me see how to deploy pinned mimic version in this repo instead of constantly changing the git revision in https://github.com/rackerlabs/autoscaling-chef |
|
Goin' in the queue… |
|
jenkins please run integration tests |
1 similar comment
|
jenkins please run integration tests |
This version of mimic will be installed when running integration tests
|
jenkins please run integration tests |
| @@ -10,6 +10,9 @@ class AutoscaleConfig(ConfigSectionInterface): | |||
| """ | |||
| SECTION_NAME = 'autoscale' | |||
|
|
|||
| lc_image_ref = None | |||
There was a problem hiding this comment.
Mutable staaaaate :-(
I do like the fact that this makes the data structure more regular-python-ish and less of a wrapper around a weird dictionary. But I don't like the fact that this change results in even less documentation for these two properties. It took me a minute to understand what they were supposed to be, and why this was in a "config section" at all. Can you add some :ivar: documentation to the class docstring?
Also, these are default-NULL, which I am coming to believe is always an antipattern. I wrote some about this in Python: https://glyph.twistedmatrix.com/2015/09/python-option-types.html and of course there's the option of sumtypes. When and why are these values None?
|
Once again sorry for the delay. This mostly seems fine; 👍 . I have a lot of concerns about the cloudcafe code, but most of that stuff is not really related to this PR. My general comment is maybe take a little more care in documenting parameter types and return values since not all of this stuff is very straightforward and I am betting that you will be cursing yourself in 6 months if you come back to it and are trying to remember why the twisted and blocking image-ID fetchers work differently :-). So maybe refine the names or add a little bit more docs, but otherwise it looks like it's doing the right thing for mimc. |
No problem at all 😄 . I should say a big thanks for reviewing such a messy code. I can try to understand how confusing and horrible this can look from external eyes 😕 |
|
jenkins please run integration tests |
Tests get imageRef from nova
instead of configured one. This overcomes rackerlabs/mimic#513 and allows us to use latest mimic.