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

delete node interactively #6480

Merged

Conversation

mickm3n
Copy link
Contributor

@mickm3n mickm3n commented Oct 13, 2023

For implementing #6450

Current behavior

When a user runs ockam node delete without any arguments, currently they are prompted to delete the default node. Here's a screenshot of this.

Image

Proposed changes

Instead of deleting the default node, change the behavior of this command so that it becomes interactive with the user. Note that this can only be allowed to happen if:

  • the terminal is interactive, and,
  • the --no-output or --quiet flags have not been passed, and,
  • no arguments have been passed in to the command.

When there one or more nodes could be deleted, after a user runs ockam node delete, the user will see

  • image
  • If the user doesn't select one and press Enter, the user will see
    • image
  • But if the user does select and press Enter, the user will see
    • image
    • After selecting YES
      • image

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.

@mickm3n mickm3n requested a review from a team as a code owner October 13, 2023 05:05
@mickm3n mickm3n force-pushed the mickm3n/delete-node-interactive branch from e832ca1 to ecbd927 Compare October 13, 2023 05:07
@mickm3n
Copy link
Contributor Author

mickm3n commented Oct 13, 2023

@nazmulidris I don't know how to make node deletion fail, so I haven't tested the failed scenario. If you have any advice, I could try it.

Any advice is welcome!

@mrinalwadhwa mrinalwadhwa added the hacktoberfest Apply to issues you want contributors to help with label Oct 13, 2023
@nazmulidris
Copy link
Contributor

I don't know how to make node deletion fail, so I haven't tested the failed scenario. If you have any advice, I could try it. Any advice is welcome!

@adrianbenavides Any thoughts on this?

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.

@nazmulidris I don't know how to make node deletion fail, so I haven't tested the failed scenario. If you have any advice, I could try it.

The easiest way would be to manually delete the pid file in the node's state directory before deleting it through the command.

@mickm3n
Copy link
Contributor Author

mickm3n commented Oct 14, 2023

Thanks for your review. I'll check.

@mickm3n
Copy link
Contributor Author

mickm3n commented Oct 16, 2023

Hi @adrianbenavides, I fix all the problems in your feedback. Please help review again 🙏

I successfully triggered the failed condition by modifying the pid in the node's pid file.
image

However, if I remove the PID file, the result will be succeeded, but the node process will remain alive. I have no idea how to trace the problem; maybe we should create an issue.

@adrianbenavides
Copy link
Member

adrianbenavides commented Oct 16, 2023

Hi @adrianbenavides, I fix all the problems in your feedback. Please help review again 🙏

I successfully triggered the failed condition by modifying the pid in the node's pid file. image

Yeah, that's right, you had to modify it, not remove it. Removing the PID file is expected to have no effect when removing the node, my mistake!

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.

I've left some more comments. Looking pretty great so far!

@mickm3n mickm3n force-pushed the mickm3n/delete-node-interactive branch from 1fd95c6 to ec56143 Compare October 16, 2023 09:39
@mickm3n
Copy link
Contributor Author

mickm3n commented Oct 16, 2023

@adrianbenavides, I appreciate your feedback. You saved me from the Option and iterator hell. I learned a lot. I fixed all of them; please help review them again. Many thanks!

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.

Looks fantastic and ready to merge! Thank you so much for this contribution @mickm3n, will get merged soon 🥳

Copy link
Contributor

@nazmulidris nazmulidris left a comment

Choose a reason for hiding this comment

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

Looks great to me! 🎉

@adrianbenavides adrianbenavides added this pull request to the merge queue Oct 17, 2023
Merged via the queue into build-trust:develop with commit 2a2843e Oct 17, 2023
26 checks passed
This was referenced Oct 18, 2023
@adrianbenavides adrianbenavides added the HACKTOBERFEST-ACCEPTED To be used when a PR is ready to merge or after it's merged label Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest Apply to issues you want contributors to help with HACKTOBERFEST-ACCEPTED To be used when a PR is ready to merge or after it's merged
Projects
None yet
4 participants