-
Notifications
You must be signed in to change notification settings - Fork 65
Description
Is your refactoring request related to a problem? Please describe.
In #2508, I added a case where we need to use a genuine TEdgeConfig
as input (specifically TEdgeConfigReaderService
) rather than simple values (e.g. String
, u32
, etc.). This motivated the creation of TEdgeConfigRepository::load_toml_str
:
thin-edge.io/crates/common/tedge_config/src/tedge_config_cli/tedge_config_repository.rs
Lines 69 to 72 in 65ab413
pub fn load_toml_str(toml: &str) -> TEdgeConfig { | |
let dto = super::figment::extract_from_toml_str(toml).unwrap(); | |
TEdgeConfig::from_dto(&dto, &TEdgeConfigLocation::default()) | |
} |
Currently the use of this function is limited to places where it has to be used, but the use could be much more widespread. This would allow the unit tests to better mimic how a user would interact with tedge. There are certain cases where we currently have very lengthy sections of code devoted to creating configurations just for testing purposes that could be removed as a result, such as
thin-edge.io/crates/extensions/c8y_mapper_ext/src/config.rs
Lines 51 to 93 in 65ab413
#[allow(clippy::too_many_arguments)] | |
pub fn new( | |
config_dir: PathBuf, | |
logs_path: Utf8PathBuf, | |
data_dir: DataDir, | |
tmp_dir: Arc<Utf8Path>, | |
device_id: String, | |
device_topic_id: EntityTopicId, | |
device_type: String, | |
service: TEdgeConfigReaderService, | |
c8y_host: String, | |
tedge_http_host: Arc<str>, | |
topics: TopicFilter, | |
capabilities: Capabilities, | |
auth_proxy_addr: Arc<str>, | |
auth_proxy_port: u16, | |
auth_proxy_protocol: Protocol, | |
mqtt_schema: MqttSchema, | |
enable_auto_register: bool, | |
) -> Self { | |
let ops_dir = config_dir.join("operations").join("c8y"); | |
Self { | |
config_dir, | |
logs_path, | |
data_dir, | |
device_id, | |
device_topic_id, | |
device_type, | |
service, | |
ops_dir, | |
tmp_dir, | |
c8y_host, | |
tedge_http_host, | |
topics, | |
capabilities, | |
auth_proxy_addr, | |
auth_proxy_port, | |
auth_proxy_protocol, | |
mqtt_schema, | |
enable_auto_register, | |
} | |
} |
Currently, the thing blocking the tests from using this is the device ID handling. We infer the device id from the device certificate file on disk, and as such if we were to inject a tedge.toml
file, this would require the creation of a certificate. It doesn't make sense to add this complexity to the tests, so we would need some sort of mitigation for this. I'm not entirely sure how this would work, but figment::value::magic has some examples of how metadata generated at the time of reading the file might be injected into a deserializable value.
Describe alternatives you've considered
The existing test code works fairly well, and is unlikely to cause us any issues per se, but it would be nice to test "closer to the user" (i.e. with a toml configuration rather than a alternative route to generating internal config structs directly).
Additional context
It's also been pointed out by @didier-wenzek here that tedge config get device.id
already works in a surprising way (i.e. it is completely useless on child devices), so this could maybe also be addressed (although I think the scope of that problem is largely separate to the scope of this issue.