-
Notifications
You must be signed in to change notification settings - Fork 152
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
feat(rename): Rename Nodes #841
Conversation
@jamaljsr here is the code Edit: the last commit fixed the issue of the ports undefined error. Although the visual connector of the node to the backend has disappeared as shown below. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for working on this. When testing it, I found a few functional issues.
polar-node-rename.mp4
- When the network is started and I enter a new name then click Save, the network is stopped, but the node isn't renamed. I have to click Save a second time to rename the node.
- After the rename, the node's open channels are no longer displayed.
- The file paths to the cert & macaroons are still showing the old name. We should rename this folder as well.
- I think it would be better to use a modal to have the user type the new name in so that we can access this from the right-click menu on each node.
- Rename is not available for the Bitcoin Core and TAP nodes. Shouldn't this be supported for all node types?
Thank so much for the review. I am going to address all these. I have a question though, about Number 4, should we remove the button and simply have the modal accessible through the right click menu or should we have both? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #841 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 141 143 +2
Lines 4663 4810 +147
Branches 903 933 +30
==========================================
+ Hits 4663 4810 +147 ☔ View full report in Codecov by Sentry. |
To be consistent with the other modals, I think you should remove the sidebar dropdown menu and add a new "Rename Node" button between the "Restart" and "Advanced Options" buttons in the Actions tab. |
|
Hey @Jem256, for each node, there is a folder on the host machine that contains the node's data. It's located at |
how can I access this dir through the code to update the name? I can't seem to figure that out |
The DockerService.ensureDirs is currently the method that initially creates this dir when it starts the containers. This seems like the most appropriate place to rename the dir when the node is renamed. You can call it from the stores using |
Hey @Jem256 please let me know when you feel this is ready for review and I'll take a look at it. |
Hey Jamal, It should be ready sometime this week. Finishing up a few things today. I'll ping you when it's ready |
@jamaljsr please take a look and let me know what you think. The channel links are not yet working but I've tried to make a lot of other things work and would like to know your thoughts on my approach. The other thing I would like to highlight is that in case someone tries to rename the bitcoin nodes, the path to the node can't be updated because it doesn't have that property. Also on the issue of links, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for working on this feature. It has been requested many times in the past. You are off to a great start. I've left a bunch of feedback on where I see room for improvement.
One strange issue I noticed when testing was that I couldn't select the text in the input field of the modal. Instead, it was dragging the node on the canvas underneath the modal. I'm not sure how that's even possible but my hunch is it's related to where you have the Modal component being mounted. I mentioned some details about this in a comment below.
polar-rename.mp4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for the additional updates. This is looking really good. I found a few remaining issues, but this should be complete once these are fixed.
The last things remaining would be to rebase this branch on master
and add/update the unit tests to fix the failures and get the coverage back up. Testing all of the code paths can be pretty tricky sometimes so please let me know if you need any assistance with this.
{node?.status === Status.Started ? ( | ||
<Alert type="warning" message={l('alert')} /> | ||
) : null} | ||
<Form.Item name="newNodeName" label={l('label')}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that we don't have any validation here, so entering weird characters like "alice : / ` asd" puts the network in a totally broken state. You cannot even delete the network after doing that.
Here's how to restrict the input to letters, numbers, -
, and _
. This
<Form.Item
name="newNodeName"
label={l('label')}
rules={[
{ required: true, message: l('cmps.forms.required') },
{ pattern: /^[a-zA-Z0-9_-]+$/, message: l('invalidName') },
]}
>
This invalidName
string in en-US.json
can be "Can only contain letters, numbers, underscores, and dashes.."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates. I've confirmed that all of the prior issues have been resolved.
I did discover one more problem which I described below.
The new code also needs unit test coverage. Are you able to work on that?
Thanks for the review. I'll work on the stated problem today. I also want to work on the unit tests over the weekend. However, is there a way to test coverage locally? Or for this particular branch? |
Yes, you can see coverage locally by running |
This reverts commit a19a2ad.
@Jem256 Thanks so much for getting this new feature 99% done. I'm planning a new release in the coming days and wanted to make sure this gets in it. So I've rebased this branch and added a commit with unit tests bring the coverage back to 100%. I also found a few edge cases that needed fixing. I am going to merge this once the CI passes. Thank you again for the contribution. |
@jamaljsr thank you so much for this and for your patience and support to getting this feature this far. |
Closes #388
Description
Adds a rename node feature that allows users to rename a node.
Screenshots