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

feat(rename): Rename Nodes #841

Merged
merged 24 commits into from
Jun 20, 2024
Merged

feat(rename): Rename Nodes #841

merged 24 commits into from
Jun 20, 2024

Conversation

Jem256
Copy link
Contributor

@Jem256 Jem256 commented Mar 1, 2024

Closes #388

Description

Adds a rename node feature that allows users to rename a node.

Screenshots

Screenshot from 2024-03-07 11-06-34

@Jem256 Jem256 changed the title feat(rename): add rename button feat(rename): Rename Nodes Mar 1, 2024
@Jem256
Copy link
Contributor Author

Jem256 commented Mar 10, 2024

@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.
Screenshot from 2024-03-11 00-45-33

@jamaljsr
Copy link
Owner

The code currently on Github is missing the lightning.renameNode() function and is throwing an error, so I don't see the issue you're having.

image

@Jem256
Copy link
Contributor Author

Jem256 commented Mar 11, 2024

The code currently on Github is missing the lightning.renameNode() function and is throwing an error, so I don't see the issue you're having.

image

I've fixed all the issues. I kindly request for a review of my PR

@Jem256 Jem256 marked this pull request as ready for review March 11, 2024 14:21
Copy link
Owner

@jamaljsr jamaljsr left a 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
  1. 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.
  2. After the rename, the node's open channels are no longer displayed.
  3. The file paths to the cert & macaroons are still showing the old name. We should rename this folder as well.
  4. 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.
  5. Rename is not available for the Bitcoin Core and TAP nodes. Shouldn't this be supported for all node types?

@Jem256
Copy link
Contributor Author

Jem256 commented Mar 11, 2024

Thanks so much for working on this. When testing it, I found a few functional issues.
polar-node-rename.mp4

1. 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.

2. After the rename, the node's open channels are no longer displayed.

3. The file paths to the cert & macaroons are still showing the old name. We should rename this folder as well.

4. 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.

5. 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-commenter
Copy link

codecov-commenter commented Mar 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (00197b7) to head (027079e).

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.
📢 Have feedback on the report? Share it here.

@jamaljsr
Copy link
Owner

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?

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.

@Jem256
Copy link
Contributor Author

Jem256 commented Mar 25, 2024

Screenshot from 2024-03-25 14-03-57
Hey @jamaljsr do you have an idea why this is happening? when the app is restarted, the paths update to the new node name but the node also shows this error.

@jamaljsr
Copy link
Owner

Hey @Jem256, for each node, there is a folder on the host machine that contains the node's data. It's located at ~/.polar/networks/<network-id>/volumes/<implementation>/<node-name>/. It looks like Polar isn't renaming this dir when the node is renamed.

@Jem256
Copy link
Contributor Author

Jem256 commented Mar 25, 2024

~/.polar/networks//volumes///

how can I access this dir through the code to update the name? I can't seem to figure that out

@jamaljsr
Copy link
Owner

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 injections.dockerService.

@Jem256 Jem256 marked this pull request as draft April 2, 2024 09:00
@jamaljsr
Copy link
Owner

Hey @Jem256 please let me know when you feel this is ready for review and I'll take a look at it.

@Jem256
Copy link
Contributor Author

Jem256 commented Apr 24, 2024

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

@Jem256 Jem256 requested a review from jamaljsr April 24, 2024 11:49
@Jem256 Jem256 marked this pull request as ready for review April 24, 2024 11:50
@Jem256
Copy link
Contributor Author

Jem256 commented Apr 24, 2024

@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, syncChart doesn't work in updating them. But the functions updateBackendLink and updateTapBackendLink worked for those corresponding links. Do you think we have to write a separate function like updateChannelLink for the channels

Copy link
Owner

@jamaljsr jamaljsr left a 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

@Jem256 Jem256 requested a review from jamaljsr May 2, 2024 17:17
Copy link
Owner

@jamaljsr jamaljsr left a comment

Choose a reason for hiding this comment

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

This is looking amazing! Great work 👍

I just have a bit more feedback on the latest changes.

Also, I noticed this error when trying to rename a Core Lightning node. Does it work fine for you?

image

@Jem256 Jem256 requested a review from jamaljsr May 11, 2024 20:51
Copy link
Owner

@jamaljsr jamaljsr left a 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')}>
Copy link
Owner

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.."

Copy link
Owner

@jamaljsr jamaljsr left a 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?

@Jem256
Copy link
Contributor Author

Jem256 commented Jun 6, 2024

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?

@jamaljsr
Copy link
Owner

jamaljsr commented Jun 6, 2024

Yes, you can see coverage locally by running yarn test:ci. The output will show you which files have uncovered lines. Afterwards, you can also open the coverage/lcov-report/index.html file in a browser to view the same results in a nicer html format.

@jamaljsr
Copy link
Owner

@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 jamaljsr merged commit f336eaf into jamaljsr:master Jun 20, 2024
5 checks passed
@Jem256
Copy link
Contributor Author

Jem256 commented Jun 20, 2024

@jamaljsr thank you so much for this and for your patience and support to getting this feature this far.

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.

Feature Request: Rename nodes
3 participants