fix: re-enable connect checks for aws and azure#3654
Conversation
It was disabled in f40a93c, possibly by mistake because we set a variable used only when connecting to c8y. However, aside from setting that variable we emit a warning if the check is not successful, which is desired. As such, connection checks for these clouds were reenabled. Signed-off-by: Marcel Guzik <marcel.guzik@cumulocity.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! 🚀 New features to boost your workflow:
|
Robot Results
|
rina23q
left a comment
There was a problem hiding this comment.
Definitely a good catch and truly connection check was skipped when I built a binary by only azure feature (cargo build --no-default-features --features "azure" )
$ sudo ./target/debug/tedge connect az
connect to Azure cloud.:
device id: rina0010
cloud profile: <none>
cloud host: thin-edge-test-hub.azure-devices.net:8883
auth type: Certificate
certificate file: /etc/tedge/device-certs/tedge-certificate.pem
cryptoki: off
bridge: mosquitto
service manager: systemd
proxy: Not configured
Restarting mosquitto... ✓
Waiting for mosquitto to be listening for connections... ✓
Enabling tedge-mapper-az... ✓
Verifying device is connected to cloud... ✓ was missing from the output, so it's skipped.
And after removing feature flags as in this PR, the message is back.
| } | ||
|
|
||
| #[cfg(feature = "c8y")] | ||
| let mut connection_check_success = true; |
There was a problem hiding this comment.
I guess that this unused variable warning was misleading and caused the mistake.
warning: variable `connection_check_success` is assigned to, but never used
--> crates/core/tedge/src/cli/connect/command.rs:231:17
|
231 | let mut connection_check_success = true;
| ^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: consider using `_connection_check_success` instead
= note: `#[warn(unused_variables)]` on by default
However, I couldn't come up with a good refactoring idea to solve this... connection check has to be done before the mapper starts, and tenant_matches_configured_url() needs a connection to the cloud. So, we can't remove the connection check condition here.
if !use_basic_auth && !self.offline_mode && connection_check_success {
let _ = self.tenant_matches_configured_url(tedge_config).await;
}Though, honestly, this one can be simplified to this. tenant_matches_configured_url() checks basic_auth and offline_mode inside the function. But this is unrelated to this PR's purpose.
if connection_check_success {
let _ = self.tenant_matches_configured_url(tedge_config).await;
}There was a problem hiding this comment.
You can ignore the last point, I'm preparing a refactoring PR regarding of the usage of use_basic_auth condition.
Proposed changes
Reenable connect checks for aws and azure.
It was disabled in f40a93c, possibly by mistake because we set a variable used only when connecting to c8y. However, aside from setting that variable we emit a warning if the check is not successful, which is desired. As such, connection checks for these clouds were reenabled.
Types of changes
Paste Link to the issue
Checklist
just prepare-devonce)just formatas mentioned in CODING_GUIDELINESjust checkas mentioned in CODING_GUIDELINESFurther comments