Add integration test for lp-1920939 #891
Conversation
In canonical#856 we added the ability to use partprobe instead of blockdev for reading partitions. Test that partprobe succeeds where blockdev fails. Also add a mechanism to our integration tests to allow a callable to be called between `lxc init` and `lxc start`
There was a problem hiding this comment.
This looks good, thanks! I have some nittish comments about kwargs naming/presence inline: given these are important APIs, I think they're worth addressing (but I'll Approve and leave it up to you).
More broadly, I feel like we're starting to really push the sensible limits of kwargs in this part of the codebase. I wonder if introducing a parameter object pattern might help here: we would have a single layer which would translate marks into a Python object, which we'd then pass around: the various layers of our stack would access their parameters (e.g. params.image_id) but pass the same object around. Not for this PR, of course, but I suspect this would be easier to follow but, again, I'll leave it up to you!
| @@ -292,7 +292,7 @@ def _mount_source(instance: LXDInstance): | |||
| ).format(**format_variables) | |||
| subp(command.split()) | |||
|
|
|||
| def _perform_launch(self, launch_kwargs): | |||
| def _perform_launch(self, launch_kwargs, **kwargs): | |||
There was a problem hiding this comment.
I think we can just specify lxd_setup=None here? (Or =lambda _: None if you're feeling spicy, or perhaps id, which is effectively a noop: https://docs.python.org/3/library/functions.html#id)
There was a problem hiding this comment.
I was hoping to keep the method arguments the same here (Liskov Substitution Principle?) which is why I went with the more generic **kwargs.
(Same comment for the other comment on the other _perform_launch).
| @@ -142,12 +142,12 @@ def _get_initial_image(self): | |||
| except (ValueError, IndexError): | |||
| return image.image_id | |||
|
|
|||
| def _perform_launch(self, launch_kwargs): | |||
| def _perform_launch(self, launch_kwargs, **kwargs): | |||
There was a problem hiding this comment.
I don't think we need to take **kwargs here: on non-LXD platforms, we should have skipped any tests which are passing lxd_setup before we get to this point.
| pycloudlib_instance = self.cloud_instance.launch(**launch_kwargs) | ||
| return pycloudlib_instance | ||
|
|
||
| def launch(self, user_data=None, launch_kwargs=None, | ||
| settings=integration_settings): | ||
| settings=integration_settings, **kwargs): |
There was a problem hiding this comment.
Would naming this in a non-default fashion make it clearer that it's passed through unmodified (e.g. perform_launch_kwargs)? I don't think we want to be conflating other things into this parameter in future.
There was a problem hiding this comment.
I'm wary of pushing LXD-specific stuff up into the base class. I've always thought of **kwargs (especially when named **kwargs) to serve exactly this purpose when dealing with inheritance. Got some instance-specific info you need to pass but want to keep a consistent API and aren't keen on adding more abstraction? Stuff it into kwargs. I thought that was a well-trodden python paradigm, but I could be wrong.
Proposed Commit Message
Additional Context
Test Steps
Run the test
Checklist: