fix: Remove hardcoded /var/lib/cloud hotplug path #4940
Conversation
The cloud dir is configurable, so we shouldn't be hardcoding the hotplug.enabled file location
| enable_hotplug(mocks.m_init, "net") | ||
|
|
||
| assert [ | ||
| call([EventType.HOTPLUG]) | ||
| ] == mocks.m_init.datasource.get_supported_events.call_args_list | ||
| assert [call()] == m_read_hotplug_enabled_file.call_args_list | ||
| m_read_hotplug_enabled_file.assert_called_once() |
There was a problem hiding this comment.
would there be any value to checking not only .assert_called_once() but also the call list since read_hotplug_enabled_file() now accepts an argument?
There was a problem hiding this comment.
I don't think so. We're mocking that argument in all of the tests anyway, so I don't think we really get any signal from introspecting the argument.
There was a problem hiding this comment.
D'oh. My suggestion on the previous PR about locating this hard-coded path in settings was short-sighted. +1. I should have thought about that aspect in review, especially given Brett's ongoing work on other dynamic path concerns with the configurable /run/cloud-init work in #4820.
| self.target_file = target_file | ||
|
|
||
| def get_cpath(self, *args): | ||
| return self.target_file |
There was a problem hiding this comment.
Probably overkill, but none of these tests assert we are specifically looking at hotplug.enabled in get_cpath. So, our implementation of read_hotplug_enabled_file could change to get_cpath("warnings") with no test failures.
Can we assert we are validating the specific hotplug.enabled lookup in Paths somehow in these tests. Maybe just something like:
--- a/tests/unittests/test_util.py
+++ b/tests/unittests/test_util.py
@@ -3231,6 +3231,9 @@ class MockPath:
self.target_file = target_file
def get_cpath(self, *args):
+ assert args == (
+ "hotplug.enabled",
+ ), f"Invalid get_cpath argument {args}"
return self.target_file
The cloud dir is configurable, so we shouldn't be hardcoding the hotplug.enabled file location
The cloud dir is configurable, so we shouldn't be hardcoding the hotplug.enabled file location
The cloud dir is configurable, so we shouldn't be hardcoding the hotplug.enabled file location
The cloud dir is configurable, so we shouldn't be hardcoding the hotplug.enabled file location
Proposed Commit Message
Additional Context
https://jenkins.canonical.com/server-team/view/cloud-init/job/cloud-init-integration-focal-lxd_vm/407/testReport/junit/tests.integration_tests.test_paths/TestHonorCloudDir/test_honor_cloud_dir/
Merge type