Skip to content
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

Update CLI documentation for status and reset commands (resolves #4771) #8070

Closed
wants to merge 1 commit into from

Conversation

Wryhder
Copy link
Contributor

@Wryhder Wryhder commented May 24, 2024

Current behavior

The docs for the status and reset commands are missing code examples and a long about section.

Proposed changes

Resolves #4771

The modifications are as suggested in this comment.

Checks

  • All commits in this Pull Request are signed and Verified by Github.
  • All commits in this Pull Request follow the Ockam commit message convention.
  • There are no Merge commits in this Pull Request. Ockam repo maintains a linear commit history. We merge Pull Requests by rebasing them onto the develop branch. Rebasing to the latest develop branch and force pushing to your Pull Request branch is okay.
  • I have read and accept the Ockam Community Code of Conduct.
  • I have read and accepted the Ockam Contributor License Agreement by adding my Git/Github details in a row at the end of the CONTRIBUTORS.csv file in a separate pull request to the build-trust/ockam repository. The easiest way to do this is to edit the CONTRIBUTORS.csv file in the github web UI and create a separate Pull Request, this will mark the commit as verified.

@Wryhder Wryhder force-pushed the update-cli-docs branch 2 times, most recently from 588fddd to f244fc4 Compare June 28, 2024 15:24
@Wryhder Wryhder force-pushed the update-cli-docs branch from f244fc4 to 3f0fe6c Compare July 9, 2024 13:35
@Wryhder Wryhder closed this Aug 3, 2024
@Wryhder Wryhder reopened this Aug 3, 2024
@Wryhder
Copy link
Contributor Author

Wryhder commented Aug 4, 2024

@adrianbenavides Hi! Could you please address these?

  1. Is this expected behaviour? Some information isn't included in the output of ockam status until after some other command is executed. For example, I ran ockam message send hello --to /node/n1/service/uppercase successfully after starting the node, but only after running ockam service list --at n1 did the Transport, Secure Channel, and Services information populate. Also, only after running ockam space list was subscription info included in the status output.
  2. Please help with any needed clarifications for this comment.
  3. Please review the long about for status in my commit. Is that useful information or unnecessary duplication? Should anything in particular be elaborated on, and in what way?
  4. When might it be useful or necessary to do a reset (asides maybe discarding after initially testing or trying things out)?

@Wryhder Wryhder marked this pull request as ready for review August 4, 2024 16:20
@Wryhder Wryhder requested a review from a team as a code owner August 4, 2024 16:20
@adrianbenavides
Copy link
Member

Please review the long about for status in my commit. Is that useful information or unnecessary duplication? Should anything in particular be elaborated on, and in what way?

I think I answered this in the review. Let me know if you have any further questions!

When might it be useful or necessary to do a reset (asides maybe discarding after initially testing or trying things out)?

The reset command should be advertised as a dangerous / "use with caution" command really (we should display this in the command docs somehow), as it's used to remove all the local state permanently. Usually, the user will use more granular delete commands (e.g. node delete, identity delete, space delete, ...). But, yeah, as you said, the reset command is mostly used in examples, or while developing, to clean up things easily and start from scratch.

@Wryhder
Copy link
Contributor Author

Wryhder commented Oct 14, 2024

Please review the long about for status in my commit. Is that useful information or unnecessary duplication? Should anything in particular be elaborated on, and in what way?

I think I answered this in the review. Let me know if you have any further questions!

When might it be useful or necessary to do a reset (asides maybe discarding after initially testing or trying things out)?

The reset command should be advertised as a dangerous / "use with caution" command really (we should display this in the command docs somehow), as it's used to remove all the local state permanently. Usually, the user will use more granular delete commands (e.g. node delete, identity delete, space delete, ...). But, yeah, as you said, the reset command is mostly used in examples, or while developing, to clean up things easily and start from scratch.

@adrianbenavides Thanks a lot for your review! And apologies for the delay on this PR. I'll resume work on it within the next couple of days.

@Wryhder Wryhder closed this Nov 3, 2024
@Wryhder Wryhder reopened this Nov 3, 2024
@Wryhder
Copy link
Contributor Author

Wryhder commented Nov 3, 2024

@adrianbenavides I've made the suggested modifications. Please take a look again when you get the chance. Thank you!

@Wryhder
Copy link
Contributor Author

Wryhder commented Nov 4, 2024

The reset command should be advertised as a dangerous / "use with caution" command really (we should display this in the command docs somehow), as it's used to remove all the local state permanently. Usually, the user will use more granular delete commands (e.g. node delete, identity delete, space delete, ...). But, yeah, as you said, the reset command is mostly used in examples, or while developing, to clean up things easily and start from scratch.

@adrianbenavides What about a superscript-type tag like this (saying "unsafe" or similar), in addition to the warning-level notification in the doc itself?

doc_notification_tag

@Wryhder Wryhder changed the title [WIP] Update CLI documentation for status and reset commands (resolves #4771) Update CLI documentation for status and reset commands (resolves #4771) Nov 4, 2024
@adrianbenavides
Copy link
Member

Awesome, thanks @Wryhder ! I'll take a look as soon as possible 🙏

@adrianbenavides
Copy link
Member

@adrianbenavides What about a superscript-type tag like this (saying "unsafe" or similar), in addition to the warning-level notification in the doc itself?

That would be nice indeed. We do something similar with the "preview" tag (here and here) if that's useful.

Copy link
Member

@adrianbenavides adrianbenavides left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Wryhder I've left some comments to improve the output a little bit but it looks nice, thanks for taking the time with this!

One final recommendation: make sure you run cargo clippy and cargo fmt and squash all the commits into a single one once you have everything ready.

@Wryhder Wryhder closed this Nov 11, 2024
@Wryhder Wryhder deleted the update-cli-docs branch November 11, 2024 22:51
@Wryhder Wryhder restored the update-cli-docs branch November 12, 2024 00:30
@Wryhder Wryhder reopened this Nov 12, 2024
@Wryhder Wryhder closed this Nov 12, 2024
@Wryhder Wryhder reopened this Nov 12, 2024
@Wryhder
Copy link
Contributor Author

Wryhder commented Nov 12, 2024

@Wryhder I've left some comments to improve the output a little bit but it looks nice, thanks for taking the time with this!

One final recommendation: make sure you run cargo clippy and cargo fmt and squash all the commits into a single one once you have everything ready.

@adrianbenavides I've made the modifications, run cargo clippy and cargo fmt, and squashed the commits. Please take a look when you get the chance. Thank you for your help with this!

@Wryhder
Copy link
Contributor Author

Wryhder commented Nov 12, 2024

@adrianbenavides What about a superscript-type tag like this (saying "unsafe" or similar), in addition to the warning-level notification in the doc itself?

That would be nice indeed. We do something similar with the "preview" tag (here and here) if that's useful.

@adrianbenavides This should be in a new PR, yes?

Copy link
Member

@adrianbenavides adrianbenavides left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @Wryhder just left some minor changes and we are good to merge!

@adrianbenavides
Copy link
Member

@Wryhder there are a couple of CI jobs failing:

  • the commit linter is complaining about the CLI word being in uppercase. You can just amend it using cli instead.
  • the editorconfig linter is complaining about a couple of missing newlines at the end of the files. Take a look here for more details

@Wryhder Wryhder force-pushed the update-cli-docs branch 2 times, most recently from a63401c to 61b884b Compare November 18, 2024 22:01
@Wryhder
Copy link
Contributor Author

Wryhder commented Nov 18, 2024

@Wryhder there are a couple of CI jobs failing:

  • the commit linter is complaining about the CLI word being in uppercase. You can just amend it using cli instead.
  • the editorconfig linter is complaining about a couple of missing newlines at the end of the files. Take a look here for more details

@adrianbenavides I added the final newlines and modified the commit message while squashing the commits. I've successfully changed the commit message but the newlines seem to have disappeared. I'm trying to figure out if it's my editor or what else.

@Wryhder Wryhder force-pushed the update-cli-docs branch 2 times, most recently from 6a4d72f to f441c40 Compare November 19, 2024 10:53
@Wryhder
Copy link
Contributor Author

Wryhder commented Nov 20, 2024

@adrianbenavides There are still a couple of CI jobs failing. One is about my commit not being signed and verified. Wanted to let you know I'll take a look by weekend.

@Wryhder Wryhder force-pushed the update-cli-docs branch 2 times, most recently from 439b53d to 7357bb2 Compare November 23, 2024 18:39
@Wryhder
Copy link
Contributor Author

Wryhder commented Nov 23, 2024

@adrianbenavides I think this is good to go now. Please confirm when you get the chance. Thanks very much for your time and help with this.

@adrianbenavides
Copy link
Member

Thanks @Wryhder for taking care of that! It all looks good now. The failing CI job is not related to this PR, and will get fixed on develop soon. We'll merge your PR after that is fixed. Thanks again for the time you put into this 🙏

@adrianbenavides
Copy link
Member

Hey @Wryhder ! I've created a PR here with your commit fixing the conflicts for you. I've cherry-picked your commit, so you will get all the credit for your contribution.

Sorry again for the delay. We've been pretty busy working in the latest release and I didn't have the time to come back to this.

I'm closing this PR in favor of the other.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update CLI documentation for run, status and reset commands
2 participants