[CIT-300] Add --test option to tedge connect#218
Merged
rina23q merged 3 commits intothin-edge:mainfrom May 7, 2021
Merged
Conversation
Highlights: - Add `--test` option to tedge connect az/c8y - check_connection() returns Err if fails (previously Ok) - `--test` returns exit code 1 if check_connection() fails - Without `--test` returns exit code 0 if check_connection() fails (warning, no change) Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! 🚀 New features to boost your workflow:
|
PradeepKiruvale
approved these changes
May 7, 2021
Contributor
PradeepKiruvale
left a comment
There was a problem hiding this comment.
Code changes LGTM.
makr11st
approved these changes
May 7, 2021
tedge/src/cli/connect/command.rs
Outdated
Comment on lines
+78
to
+87
| match self.check_connection() { | ||
| Ok(()) => Ok(()), | ||
| _ => { | ||
| println!( | ||
| "Warning: Bridge has been configured, but {} connection check failed.\n", | ||
| self.cloud.as_str() | ||
| ); | ||
| Ok(()) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
This match is a little hard to read, how about:
Suggested change
| match self.check_connection() { | |
| Ok(()) => Ok(()), | |
| _ => { | |
| println!( | |
| "Warning: Bridge has been configured, but {} connection check failed.\n", | |
| self.cloud.as_str() | |
| ); | |
| Ok(()) | |
| } | |
| } | |
| if self.check_connection().is_err() { | |
| println!( | |
| "Warning: Bridge has been configured, but {} connection check failed.\n", | |
| self.cloud.as_str() | |
| ); | |
| } | |
| Ok(()) |
Member
Author
There was a problem hiding this comment.
Good suggestion! I'll change it to this.
tedge/src/cli/connect/command.rs
Outdated
| println!("Warning: Bridge has been configured, but Cumulocity connection check failed.\n",); | ||
| Ok(()) | ||
| Err(ConnectError::NoPacketsReceived { | ||
| cloud: "Cumulocity".to_string(), |
Contributor
There was a problem hiding this comment.
Just to stay consistent with other parts of the code.
Suggested change
| cloud: "Cumulocity".to_string(), | |
| cloud: "Cumulocity".into(), |
tedge/src/cli/connect/command.rs
Outdated
| Ok(()) | ||
| } | ||
| _err => Err(ConnectError::NoPacketsReceived { | ||
| cloud: "Azure".to_string(), |
Contributor
There was a problem hiding this comment.
Suggested change
| cloud: "Azure".to_string(), | |
| cloud: "Azure".into(), |
Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>
10 tasks
12 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Highlights:
--testoption to tedge connect az/c8y--testreturns exit code 1 if check_connection() fails--testreturns exit code 0 if check_connection() fails (warning, no change)Signed-off-by: Rina Fujino 18257209+rina23q@users.noreply.github.com
Proposed changes
Introduce
--testoption totedge connect az/c8y.Usage:
sudo tedge connect c8y --test
sudo tedge connect az --test
Returns
0 (successful)
1 (failed)
It doesn't diagnose why it failed. It could be several reasons, mosquitto is not running, bridge doesn't exists, configuration in bridge file is incorrect, the cloud server is down, the device has no connection to the internet...
Potential improvements are adding small diagnoses. For example, "mosquitto is not running" and "bridge file doesn't exit" can be covered by the existing code.
Types of changes
What types of changes does your code introduce to
thin-edge.io?Put an
xin the boxes that applyPaste Link to the issue
Checklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.cargo fmtas mentioned in CODING_GUIDELINEScargo clippyas mentioned in CODING_GUIDELINESFurther comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...