Main tedge config refactoring #1916
Conversation
This builds on previous changes to TEdgeConfigDTO, adding auto generated documentation from the doc comments, as well as a simpler means to access configuration via getters and update values from both the type stored in the dto and a string representation. This also migrates the tedge command to use the updated configuration, and reformats the result of `tedge config list --doc` to be more colourful (and hopefully readable, too). Signed-off-by: James Rhodes <jarhodes314@gmail.com>
Signed-off-by: James Rhodes <jarhodes314@gmail.com>
jarhodes314
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
| /// | ||
| /// # Ok::<_, TEdgeConfigError>(()) | ||
| /// ``` | ||
| pub struct NewTEdgeConfig { |
There was a problem hiding this comment.
The module itself is called new_tedge_config, so could we not repeat New here? As long as the old module is still in use, I think it's fine for code using new config to explicitly use tedge_config::tedge_config_cli::new_tedge_config::TEdgeConfig, or if we renamed the module as well, use tedge_config::tedge_config_cli::new::TEdgeConfig.
If we want this to be available directly under tedge_config, we could rename in lib.rs: pub use self::tedge_config_cli::old_tedge_config::TEdgeConfig as NewTEdgeConfig.
There was a problem hiding this comment.
We could, although I'd prefer to keep the simple, unambiguous solution for now. I intend to update all the existing usage and completely replace OldTEdgeConfig with NewTEdgeConfig (and rename the struct and module to not contain new), albeit likely in a separate PR.
|
|
||
| #[cfg(test)] | ||
| #[allow(clippy::expect_fun_call)] | ||
| mod tests { |
There was a problem hiding this comment.
The file is a bit large, I think we can extract tests to tedge_config_cli/new_tedge_config/tests.rs? As the new_tedge_config.rs file will still be quite big with test extracted, it can remain under it's original name, instead of being moved into the directory and renamed to mod.rs.
| @@ -4,8 +4,9 @@ pub mod tedge_config_cli; | |||
| pub use self::tedge_config_cli::config_setting::*; | |||
| pub use self::tedge_config_cli::error::*; | |||
| pub use self::tedge_config_cli::models::*; | |||
| pub use self::tedge_config_cli::new_tedge_config::*; | |||
| pub use self::tedge_config_cli::old_tedge_config::*; | |||
There was a problem hiding this comment.
We're already in tedge_config crate, could we also update module names to not include tedge_config in them? There would be less typing and we could revise whether or not do we want to reexport all these items as we do now.
There was a problem hiding this comment.
I agree, although I think for this PR it's probably sensible to keep the major structural changes together, and deal with this naming issue separately.
| SettingIsNotConfigurable { | ||
| key: &'static str, | ||
| message: &'static str, | ||
| }, | ||
|
|
||
| #[error("An error occurred: {msg}")] | ||
| Other { msg: &'static str }, | ||
|
|
||
| #[error("Unrecognised configuration key '{key}'")] | ||
| WriteUnrecognisedKey { |
There was a problem hiding this comment.
I think we should use different error types for different operations. If there are different error variants for reading and writing, and write-related error will never be returned from a function doing the reading, we could have something like ReadSettingError and WriteSettingError. Would it be practical to include it in this PR or do you reckon it should be done separately?
There was a problem hiding this comment.
I think we should use different error types for different operations. If there are different error variants for reading and writing, and write-related error will never be returned from a function doing the reading, we could have something like
ReadSettingErrorandWriteSettingError. Would it be practical to include it in this PR or do you reckon it should be done separately?
I appreciate the need to put apart the read and write errors, I would prefer to see that change in a different PR this one being already quite large.
|
|
||
| #[derive(Copy, Clone, Default, Debug, Deserialize, Serialize, PartialEq, Eq)] | ||
| #[serde(transparent)] | ||
| /// A wrapper type to add implementations of [fake::Dummy] to `T` in cases where `T: !fake::Dummy` |
There was a problem hiding this comment.
Link here doesn't work. Could you also expand what this Dummy is and why we use it, ideally as a module doc but can also be here.
| @@ -536,4 +547,18 @@ mod tests { | |||
| let err = get_gid_by_name("test").unwrap_err(); | |||
| assert!(err.to_string().contains("Group not found")); | |||
| } | |||
|
|
|||
| fn current_username() -> String { | |||
| users::get_current_username() | |||
There was a problem hiding this comment.
I looked up this function, and it looks like it fetches the GID of the running process and then looks up the name of the group by the GID. Vice-versa for users. Looking up the name can fail if there is no group with this ID, but it is because we just got it off the process, in this case the function should be infallible? If this is right, then please add a comment with this info so readers know why unwrap() is fine here.
|
|
||
| pub(crate) fn get_connected_c8y_url(port: u16, host: String) -> Result<String, ConnectError> { | ||
| pub(crate) fn get_connected_c8y_url(port: u16, host: IpAddress) -> Result<String, ConnectError> { |
There was a problem hiding this comment.
As above, I think that may be incorrect because hosts can be also DNS names, but I'm not sure
There was a problem hiding this comment.
@Bravo555 is correct we need to be able to give a host name.
There was a problem hiding this comment.
If that's the case, the logic calling this that can only ever be an IpAddress is broken. I presume it's a case of us wanting to use the MQTT client address rather than the bind address? My gut feeling there is that the client address default should be the bind address (whereas the current default is just a copy of the bind address default)?
|
|
||
| /// The configuration of thin-edge | ||
| /// | ||
| /// This handles fetching values from [TedgeConfigDto](super::tedge_config_dto::TEdgeConfigDto) as well as from the system |
There was a problem hiding this comment.
I see there is some hard wrapping for most doc comments, but in some places rows are very short and in some places it looks like they're very wide. So you could some of the comments to some arbitrary threshold, but I don't think either thin-edge or rustdoc mandate it, so it's just a suggesstion. I personally wrap comments to 80 columns and use Rewrap extension for VSCode for automatic wrapping.
jarhodes314
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
So I have some concerns about simplifying TOML key names in order to accommodate env variable names. Sorry for not raising this earlier, but only now when implementing MQTT authentication, I see that I use up to 4 levels of nesting ( So I wonder whether or not do we get kind of the worst of both worlds here. The advantage of TOML is that it can be deeply hierarchical, contain objects, arrays, etc. but we restrict it two two levels only so that we can better support pretty looking environment variable names. Have we considered the option of completely disallowing underscores ( name = "Orange"
physical.color = "orange"
physical.shape = "round"
site."google.com" = true
This excess nesting sounds pretty painful, but I'd rather have that than only nesting 2 levels deep. |
There's no technical reason why there needs to only be one dot to support environment variable names, it just made the logic for translating them to the correct key much simpler as it can be done without any knowledge of what the keys are. So long as the keys are unambiguous (e.g. we don't have keys like c8y.root.cert.path and c8y.root_cert_path meaning separate things), we can map the environment variables to variables successfully. The reason why the TOML is two levels deep here is because that is what is was. I was trying to (mostly) preserve the existing If we do change the structure, we absolutely need to get the naming somewhat self consistent. From what I can tell, FWIW, my (pretty recent, but maybe not since the last |
There was a problem hiding this comment.
I still need some time to have an overall view of the impacts of the proposed changes.
Looking things independently, I saw many interesting points compared to the old version. However, I still have a feeling that this if by far too complex for the task. The configuration_keys! invocation highlights how many weirdness we have introduced over time. Dealing with all of them is a "tour de force" and I admire you ingenuity to cope with this legacy. However, I wonder if it wouldn't be simpler to give up maintaining backward compatibility on some of these points.
- Do we really need to have a
TedgeConfigandTedgeConfigDto? - Can this be simplified once the
OldTedgeConfigfully deprecated?
| // TODO add a test to make sure people don't accidentally set the wrong meta name | ||
| if let Some(note) = ty.metas.get("note") { |
There was a problem hiding this comment.
I would not bother too much here. That said I don't understand why the note is accessed using ty.metas.get("note") while the example is accessed using ty.example.
There was a problem hiding this comment.
Doku has official support for examples, so we just use #[doku(example="value")], but for notes we have to use metas (#[doku(meta(note = "A note"))] ) which is doku's mechanism for arbitrary structured metadata.
| // TODO should this be client host? or should it really be bind_address | ||
| .with_internal_opts(config.mqtt_port(), config.mqtt_client_host()) |
There was a problem hiding this comment.
This must be the mqtt.bind_address. The mqtt.client_host points to the same but can be a DNS name (therefore cannot be used as a bind address).
| // TODO should this be client host? or should it really be bind_address | |
| .with_internal_opts(config.mqtt_port(), config.mqtt_client_host()) | |
| .with_internal_opts(config.mqtt_port(), config.mqtt_bind_address()) |
| figment = { version = "0.10", features = ["test"] } | ||
| rand = "0.8.5" |
There was a problem hiding this comment.
We agreed to not specify the PATCH number, letting cargo free to get the latest.
| @@ -17,9 +17,10 @@ pub struct TEdgeConfigRepository { | |||
| pub trait ConfigRepository<T> { | |||
| type Error; | |||
| fn load(&self) -> Result<T, Self::Error>; | |||
| // TODO don't make this part of the trait. Do we even need the trait? | |||
There was a problem hiding this comment.
The trait is a vestige of a complex config setting with different configurations for root and the regular users.
It can be safely removed.
| SettingIsNotConfigurable { | ||
| key: &'static str, | ||
| message: &'static str, | ||
| }, | ||
|
|
||
| #[error("An error occurred: {msg}")] | ||
| Other { msg: &'static str }, | ||
|
|
||
| #[error("Unrecognised configuration key '{key}'")] | ||
| WriteUnrecognisedKey { |
There was a problem hiding this comment.
I think we should use different error types for different operations. If there are different error variants for reading and writing, and write-related error will never be returned from a function doing the reading, we could have something like
ReadSettingErrorandWriteSettingError. Would it be practical to include it in this PR or do you reckon it should be done separately?
I appreciate the need to put apart the read and write errors, I would prefer to see that change in a different PR this one being already quite large.
| /// A cache for the device id, to save us re-reading certificates unneccessarily | ||
| device_id: OnceCell<String>, |
There was a problem hiding this comment.
This wart is okay for now, but we might have to revise the whole idea of computed settings as the device.id. Indeed, in a containerized environment, this works only in the container running mosquitto.
|
|
||
| Ok(match key { | ||
| ReadOnly(DeviceId) => Some(self.device_id()?.to_string()), | ||
| ReadOnly(HttpAddress) => Some(self.http_address().to_string()), |
There was a problem hiding this comment.
This wart (using the MQTT bind address as the HTTP bind address) will also have to be removed in a near future.
| /// Getter for `firmware.child_update_timeout` | ||
| pub fn firmware_child_update_timeout(&self) -> Duration { | ||
| self.stored.firmware().child_update_timeout() | ||
| } |
There was a problem hiding this comment.
Why do we need a special case, here?
There was a problem hiding this comment.
This is a special case because it felt sensible for any Rust code accessing this to access this as a Duration, but tedge config needs to read and write this as a number of seconds. So when we read the string value, we get the number of seconds, but when we read the typed value, we get a Duration.
There was a problem hiding this comment.
I would prefer to address the issue at the level of TEdgeConfigDto, using #[serde(deserialize_with = "to_duration", serialize_with = "from_duration")].
There was a problem hiding this comment.
In which case we actually would need a wrapper type because it needs ToString and FromStr too for tedge config
There are two opposing forces at play here:
The first point requires we represent the absence of values, but the second requires we don't. Initially, I thought we could just have getters on One nice feature of the existing solution that I'm completely stuck on how I'd implement without code generation is the errors for when data isn't supplied. At the moment, we quote what configuration key isn't set, and this makes it really simple for consuming code to fail with a clear instructions on how to fix the problem. Of course you could specify manually in each of the relevant getters what the key name is, but then we're back to relying on someone to get the thing right in the first place. |
jarhodes314
force-pushed
the
feat/tedge-config-refactoring
branch
from
b2d0558 to
8edfdd6
Compare
jarhodes314
had a problem deploying
to
Test Pull Request
GitHub Actions
Error
jarhodes314
force-pushed
the
feat/tedge-config-refactoring
branch
from
8edfdd6 to
1c5045d
Compare
jarhodes314
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
jarhodes314
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
jarhodes314
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
jarhodes314
force-pushed
the
feat/tedge-config-refactoring
branch
from
79d6bb9 to
011c71a
Compare
jarhodes314
force-pushed
the
feat/tedge-config-refactoring
branch
from
011c71a to
55743ac
Compare
jarhodes314
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
|
I've now updated the PR to include @Bravo555's changes. The behaviour should be identical to before, with the exception of the Now we have actually got an instance of a multi-level hierarchical configuration, it seems weird to limit this to this one configuration. I think it makes sense to update the toml to be more hierarchical, especially mqtt. This change probably doesn't belong in this PR, mainly because it doesn't need to and this PR is already rather large. We could introduce a version field to indicate whether the configuration is updated or not, and manually (or possibly automatically, like I do here for outdated key detection) modify a On the subject of warnings, they're pretty coarse-grained at the moment, they don't have any knowledge of previous keys, so they will claim that any key with the right segments (e.g. I've also attempted (not in anything I've pushed to here) to investigate what it would be like to replace this with a procedural macro that would replace most of define_tedge_config! {
mqtt: {
#[tedge_config(default(function = "default_port"))]
port: NonZeroU16,
// I think this is a (slightly) useful feature for this one case.
// If mqtt.port is set, but not mqtt.client_port, mqtt.client_port
// should probably match it rather than defaulting to 1883.
#[tedge_config(default(config_path = "mqtt.port"))]
client_port: NonZeroU16,
#[tedge_config(all_or_nothing)]
client_auth: {
cert_file: Utf8PathBuf,
key_file: Utf8PathBuf,
},
},
// ...
}
fn default_port() -> NonZeroU16 {
1883.try_into().unwrap()
}It still supports all the other attributes we're using from serde and doku, I omitted them here to focus on the new attributes. Using this would be something like: let config: TEdgeConfig = ...;
assert_eq!(config.read().mqtt.port, default_port());
assert_eq!(config.read().mqtt.client_port, default_port());
match config.read().mqtt.client_auth {
Ok(Some(auth)) => println!("{}, {}", auth.cert_file, auth.key_file),
Ok(None) => println!("Client auth not set"),
// It generates an appropriate error message if the config
// is partially set
Err(e) => println!("Invalid config: {e}"),
}This solution has different trade offs. The parsing logic is mostly simpler to understand, and possible to unit test, but if the generated code is invalid, the error messages can be much more impenetrable. Usually with a declarative macro, rustc can point you at the bit of the macro that's failing, but emitting invalid syntax from a proc macro will generally give you no information at all as to where the error arose from. It would, however, simplify the code defining the configuration. I'll try and open a draft PR with those changes so you can have a look at the code, but you're more than welcome to go "no, this is even more complicated and I don't even want to consider it as something we have to support". |
jarhodes314
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
jarhodes314
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
jarhodes314
force-pushed
the
feat/tedge-config-refactoring
branch
from
40c1c17 to
d8c6d83
Compare
jarhodes314
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
Robot Results
Passed Tests
|
The excerpt is appealing and I would be really interested by a draft PR. However, we need to save our time and energy.
This can be indeed useful as several settings are closely related. We need to balance the cost with the actual help provided to users, though.
Here, I have a mix feeling to implement to implement this in procedural macro. As far as I know, this check is use in a single place. There are so many ways to get a configuration wrong that we cannot check all weird combinations. |
jarhodes314
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
jarhodes314
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
jarhodes314
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
jarhodes314
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
jarhodes314
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
|
superseded by #1936 |
Proposed changes
TLDR for people that are interested in the changes but don't want to review masses of code: almost all the changes here arise from
new_tedge_config.rs, runcargo doc -p tedge-config --opento see what the result of that is, and the screenshots below demonstrate the main user-facing changes.This is the main chunk of work associated with #1812 (sorry it's so enormous). There are still a few things to be done in tedge_config, as well as public facing documentation about the updates.
If a user uses a key with lots of dots rather than the updated key, using tedge config will look something like this (I also renamed software.plugin.default -> software.default_plugin as the word order became particularly clunky after replacing the dot with an underscore):
This is what
tedge config list --docnow outputs (assuming it's running in a tty, if stdout is piped elsewhere, the colours are disabled):All of the information shown here is derived from the doc comments, examples and notes in the various DTO structs. The colours and alignment are completely configurable, I just chose something that looked readable to me.
I haven't attempted yet to modify any crates other than tedge config and tedge.
I've attempted to add doc comments and examples for the major parts I've added. It's probably worth looking in the generated documentation (
cargo doc -p tedge_config --openwill get you there quickly if you want to try that), although I imagine people don't tend to runcargo docan awful lot.I also fixed some unrelated tests so they work on my machine.
Types of changes
Paste Link to the issue
#1812
Checklist
cargo fmtas mentioned in CODING_GUIDELINEScargo clippyas mentioned in CODING_GUIDELINESFurther comments
As I discussed in the issue, the main goal here was to create a single source of truth for a couple of reasons:
Annoyingly, to achieve this, I did have to use some significant, albeit declarative, macros. The results are, I believe, quite clear in the rustdoc output, and with the odd exception, are named completely intuitively. I've documented the macro syntax with relevant comments in the invocation of the macro.