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.

Sign up for GitHub

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

Jump to bottom

feat: provide --upnp flag for add command #1710

Merged
merged 3 commits into from
May 16, 2024
Merged

feat: provide --upnp flag for add command #1710

merged 3 commits into from
May 16, 2024

Conversation

jacderida
Copy link
Contributor

  • 8af5fa4 feat: provide --upnp flag for add command

    The --upnp flag that is available on the node is now also available on the node manager add
    command.

    The flag only applies to a node that was built with the upnp feature. Rather than also using the
    feature in the node manager, the command is documented to specify that it should only be used with
    a safenode binary that has the feature. So for now, this option will probably be used in
    conjunction with --path, to provide a node that has been built with upnp enabled.

  • 4c2ce73 fix: retain options on upgrade and prevent dup ports

    The following options that can be specified when the service is created are now retained on an
    upgrade:

    • Custom --node-port value
    • Custom --metrics-port value
    • The --home-network flag
    • The --upnp flag

    Tests were added for all of these, and in addition, a test was added for the retention of custom RPC
    ports, though these were already being retained on upgrade.

    Although the node port was being tracked as part of the listen_addr field in NodeServiceData, a
    new node_port field was explicitly added, because it's possible that a service can be upgraded
    before it starts, and the listen_addr is not assigned until the service starts.

    I also discovered the add command did not prevent specifying custom ports that were already in
    use. That was fixed too, and some tests were added.

Description

reviewpad:summary

jacderida added 2 commits May 15, 2024 17:30
@jacderida
feat: provide --upnp flag for add command
8af5fa4 
The `--upnp` flag that is available on the node is now also available on the node manager `add`
command.

The flag only applies to a node that was built with the `upnp` feature. Rather than also using the
feature in the node manager, the command is documented to specify that it should only be used with
a `safenode` binary that has the feature. So for now, this option will probably be used in
conjunction with `--path`, to provide a node that has been built with `upnp` enabled.
@jacderida
fix: retain options on upgrade and prevent dup ports
4c2ce73 
The following options that can be specified when the service is created are now retained on an
upgrade:

* Custom `--node-port` value
* Custom `--metrics-port` value
* The `--home-network` flag
* The `--upnp` flag

Tests were added for all of these, and in addition, a test was added for the retention of custom RPC
ports, though these were already being retained on upgrade.

Although the node port was being tracked as part of the `listen_addr` field in `NodeServiceData`, a
new `node_port` field was explicitly added, because it's possible that a service can be upgraded
before it starts, and the `listen_addr` is not assigned until the service starts.

I also discovered the `add` command did not prevent specifying custom ports that were already in
use. That was fixed too, and some tests were added.
github-advanced-security[bot]
github-advanced-security bot found potential problems May 15, 2024
View reviewed changes
sn_node_manager/src/add_services/tests.rs
eq(ServiceInstallCtx {
args: vec![
OsString::from("--rpc"),
OsString::from("127.0.0.1:20000"),

Check notice

Code scanning / devskim

Accessing localhost could indicate debug code, or could hinder scaling. Note test

Do not leave debug code in production
sn_node_manager/src/lib.rs
eq(ServiceInstallCtx {
args: vec![
OsString::from("--rpc"),
OsString::from("127.0.0.1:8081"),

Check notice

Code scanning / devskim

Accessing localhost could indicate debug code, or could hinder scaling. Note

Do not leave debug code in production
sn_node_manager/src/lib.rs
eq(ServiceInstallCtx {
args: vec![
OsString::from("--rpc"),
OsString::from("127.0.0.1:8081"),

Check notice

Code scanning / devskim

Accessing localhost could indicate debug code, or could hinder scaling. Note

Do not leave debug code in production
sn_node_manager/src/lib.rs
eq(ServiceInstallCtx {
args: vec![
OsString::from("--rpc"),
OsString::from("127.0.0.1:8081"),

Check notice

Code scanning / devskim

Accessing localhost could indicate debug code, or could hinder scaling. Note

Do not leave debug code in production
sn_node_manager/src/lib.rs Dismissed Show dismissed Hide dismissed
sn_node_manager/src/lib.rs
eq(ServiceInstallCtx {
args: vec![
OsString::from("--rpc"),
OsString::from("127.0.0.1:8081"),

Check notice

Code scanning / devskim

Accessing localhost could indicate debug code, or could hinder scaling. Note

Do not leave debug code in production
@joshuef joshuef added this pull request to the merge queue May 16, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 16, 2024
@jacderida jacderida added this pull request to the merge queue May 16, 2024
Merged via the queue into maidsafe:main with commit 7568040 May 16, 2024
34 checks passed
@jacderida jacderida deleted the node_man-feat-upnp_flag branch May 16, 2024 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joshuef joshuef joshuef approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jacderida @joshuef