-
Notifications
You must be signed in to change notification settings - Fork 54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor plugins by using new TEdgeConfig API #2064
Conversation
74290ed
to
24d77e3
Compare
Codecov Report
Additional details and impacted files
|
Robot Results
Passed Tests
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely this PR is going to the right direction. Added some comments to improve the code.
use tedge_config::new::ReadError; | ||
use tedge_config::new::TEdgeConfig; | ||
use tedge_config::tedge_config_cli::tedge_config_defaults::DEFAULT_FILE_TRANSFER_DIR_NAME; | ||
use tedge_config::IpAddress; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can stop using tedge_config::IpAddress
. Instead, use std::net::IpAddr
.
In fact, tedge_config::IpAddress
is no more than a wrapper of std::net::IpAddr
. https://github.com/thin-edge/thin-edge.io/blob/main/crates/common/tedge_config/src/tedge_config_cli/models/ipaddress.rs#L11
Also, new tedge_config uses IpAddr
. The default value is also now addressed by the new tedge config. https://github.com/thin-edge/thin-edge.io/blob/main/crates/common/tedge_config/src/tedge_config_cli/new.rs#L512
use tedge_config::FirmwareChildUpdateTimeoutSetting; | ||
use tedge_config::HttpBindAddressSetting; | ||
use tedge_config::HttpPortSetting; | ||
use tedge_config::new::TEdgeConfig; | ||
use tedge_config::IpAddress; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace this by std::net::IpAddr
. The same as in c8y_config_manager
.
let local_http_port = tedge_config.http.bind.port; | ||
let tmp_dir = tedge_config.tmp.path.as_std_path().to_path_buf(); | ||
let data_dir = tedge_config.data.path.as_std_path().to_path_buf(); | ||
let timeout_sec = Duration::from_secs(tedge_config.firmware.child.update.timeout.into()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't compile it, but it should work. Seconds
has implementation to convert to Duration
. https://github.com/thin-edge/thin-edge.io/blob/main/crates/common/tedge_config/src/tedge_config_cli/models/seconds.rs#L54
let timeout_sec = Duration::from_secs(tedge_config.firmware.child.update.timeout.into()); | |
let timeout_sec = tedge_config.firmware.child.update.timeout.duration(); |
use tedge_config::HttpBindAddressSetting; | ||
use tedge_config::HttpPortSetting; | ||
use tedge_config::new::ReadError; | ||
use tedge_config::new::TEdgeConfig; | ||
use tedge_config::IpAddress; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same as other comments.
let tmp_dir = tedge_config.tmp.path.as_std_path().to_path_buf(); | ||
let data_dir = tedge_config.data.path.as_std_path().to_path_buf(); | ||
let mqtt_host = tedge_config.mqtt.client.host.clone(); | ||
let mqtt_port = u16::from(tedge_config.mqtt.client.port); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor. I think using get()
looks nicer. https://doc.rust-lang.org/std/num/struct.NonZeroU16.html#method.get
@@ -34,6 +34,7 @@ assets = [ | |||
anyhow = "1.0" | |||
c8y_config_manager = { path = "../../crates/extensions/c8y_config_manager" } | |||
c8y_http_proxy = { path = "../../crates/extensions/c8y_http_proxy" } | |||
certificate = { path = "../../crates/common/certificate" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding dependency on certificate
crate just for CertificateError
is not a good idea. Add CertificateError
to lib.rs
of tedge_config
crate.
So, add this line to crates/common/tedge_config/src/lib.rs
.
pub use certificate::CertificateError;
Similar example. When you implement something using Topic
, you just need to add a dependency on tedge_mqtt_ext
and include tedge_mqtt_ext::Topic
instead of mqtt_channel::Topic
, although the actual definition of Topic
is inside of mqtt_channel
crate.
@@ -2,15 +2,14 @@ use c8y_config_manager::ConfigManagerBuilder; | |||
use c8y_config_manager::ConfigManagerConfig; | |||
use c8y_http_proxy::credentials::C8YJwtRetriever; | |||
use c8y_http_proxy::C8YHttpProxyBuilder; | |||
use certificate::CertificateError; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be as below. Look at my comment on Cargo.toml
file.
use tedge_config::CertificateError;
plugins/c8y_log_plugin/Cargo.toml
Outdated
@@ -35,6 +35,7 @@ assets = [ | |||
anyhow = "1.0" | |||
c8y_http_proxy = { path = "../../crates/extensions/c8y_http_proxy" } | |||
c8y_log_manager = { path = "../../crates/extensions/c8y_log_manager" } | |||
certificate = { path = "../../crates/common/certificate" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as my comment on c8y_configuration_plugin
.
Signed-off-by: Krzysztof Piotrowski <krzysztof.piotrowski@inetum.com>
527e285
to
fa9046e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved
let tmp_dir = tedge_config.tmp.path.as_std_path().to_path_buf(); | ||
let log_dir = tedge_config.logs.path.as_std_path().to_path_buf(); | ||
let mqtt_host = tedge_config.mqtt.client.host.clone(); | ||
let mqtt_port = u16::from(tedge_config.mqtt.client.port); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[minor] You can use get()
here as well.
Proposed changes
This PR replace old configuration model for c8y plugins with the new TEdgeConfig API.
Types of changes
Paste Link to the issue
part of #2019
Checklist
cargo fmt
as mentioned in CODING_GUIDELINEScargo clippy
as mentioned in CODING_GUIDELINESFurther comments