Doctor command for Health check #393
Conversation
Harshil-Jani
requested review from
KendallWeihe,
nitro-neal,
shamilovtim,
diehuxx,
frankhinek and
leordev
as code owners
|
Hii I have a doubt regarding how do we plan to health check on dependencies. I have added my doubt as a comment in code so that it can provide better context. Any help would be really appreciated. |
|
Nice thank you! It looks like you have a few failing tests, but I'll ping @nitro-neal and @KendallWeihe to address your questions! |
|
Can you try running the command below to get rid of test vector errors? git submodule update --init |
|
Hii @blackgirlbytes , I ran the command and it created a web5-spec submodule. But there is nothing new in my diff which I can push to fix the failing check. Am I missing something ? |
|
ahh all the test vectors actually passed (5) but since this is a pr from an outside source our reporting tool is crashing to show that. This is a bug and I'll create an issue, but we can continue as this pr does not affect test vectors added issuer - #395 |
| fn check_cli_version() -> Option<Option<String>> { | ||
| Some(Some(env!("CARGO_PKG_VERSION").to_string())) // This will return the binary version. | ||
| } |
There was a problem hiding this comment.
| fn check_cli_version() -> Option<Option<String>> { | |
| Some(Some(env!("CARGO_PKG_VERSION").to_string())) // This will return the binary version. | |
| } | |
| fn check_cli_version() -> Option<String> { | |
| Some(env!("CARGO_PKG_VERSION").to_string()) | |
| } |
can we remove the double Option and Some
There was a problem hiding this comment.
We can but the first reason why I kept it over there is because let's say you want to run individual tests then in the print function , I needed a way to check if a user has asked for a certain check or not.
Let me re-think this in some other possible way.
| Some(env_vars) | ||
| } | ||
|
|
||
| async fn check_connectivity() -> Option<bool> { |
There was a problem hiding this comment.
why do we need to check connectivity here?
for did web and did dht resolution ?
There was a problem hiding this comment.
Yes for web mainly
web5 did create web https://blackgirlbytes.com
|
fyi trying to run from my laptop: 
in both parent directory and web5_cli web5 did create jwk in the main directory it works: |
| fn check_dependencies() -> Option<HashMap<String, bool>> { | ||
| // TODO : Implement this function | ||
| // DOUBT : Are we expecting to check system dependencies or dependencies in Cargo.toml ? | ||
| // If cargo dependencies then how exactly shall we check the versions ? | ||
| None | ||
| } |
There was a problem hiding this comment.
I think we dont need this, everything should be in the cli binary
There was a problem hiding this comment.
Got it. Will skip this then.
|
if you merge back up to main the test vectors tests should pass now and give checks |
|
Following up on this PR as well @KendallWeihe @nitro-neal - Does @Harshil-Jani need to do anything else in order to close this PR? Since Hacktoberfest closes this week on Thursday, wanted to make sure all PRs are resolved so that contributors will get credit. Thank you so much! 🙏 |
|
Hi @Harshil-Jani - To ensure your PR can be approved and count towards Hacktoberfest, please address the comments above by today, latest early tomorrow before Hacktoberfest closes. Thank you so much! 🙏 |
What type of PR is this? (check all applicable)
Description
This PR add a
doctorcommand which will perform some sanity checks on well-being of the web5 cli.Related Tickets & Documents
Resolves #388
Mobile & Desktop Screenshots/Recordings
Added code snippets?
Added tests?
No tests? Add a note
Added to documentation?
No docs? Add a note
[optional] Are there any post-deployment tasks we need to perform?
[optional] What gif best describes this PR or how it makes you feel?