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 --owner arg for add cmd #1722

Merged
merged 5 commits into from
May 19, 2024
Merged

feat: provide --owner arg for add cmd #1722

merged 5 commits into from
May 19, 2024

Conversation

jacderida
Copy link
Contributor

@jacderida jacderida commented May 16, 2024 •
edited

  • 7d8517b feat: provide --owner arg for add cmd

    The --owner argument on the add command will be passed to the node service definition. It is
    also retained on upgrade. Test cases were added for each.

  • 514760b chore: reconfigure local network owner args

    The local run and local join commands had an optional --owner argument being used in testing
    scenarios to pass --owner to each safenode process. In its absence, the application code was
    checking if it was running in a CI context, and if so, it was assigning an individual owner to each
    node. This owner would be in the form "node_X", where "X" was replaced with the node number. If the
    code determined it wasn't running in CI, it would still launch the node with --owner, assigning a
    default value.

    This behaviour has been changed such that safenode will only be passed --owner if it has also
    been used on the node manager local run/join commands. This lets safenode itself provide a
    default value. Now, the local run --owner argument assigns the same owner to each safenode
    process in the local network, and a new argument, --owner-prefix, assigns an individual owner to
    each node, based on a prefix. For example, if you use --owner-prefix node, the nodes will be
    assigned owners "node_1", "node_2", and so on.

    We need to configure the CI system to specify owners where it needs to. This new setup prevents CI
    details leaking in to the application code.

  • 64638f0 ci: use owners on memcheck workflow local network

    With the new --owner-prefix argument available on the local run command and the local testnet
    action updated for this, we can use it in the memcheck workflow to specify that nodes should be
    assigned the owners "node_1", "node_2", and so on.

  • 2222787 feat: supply discord username on launchpad

    The Discord username is hooked up to the safenode --owner argument in the service definition.

  • cf3a4a4 chore: check we are in terminal before creating one

    I tried to run node-launchpad in a headless VM and it said it couldn't find a terminal. We don't
    need to find a terminal if we are already running in one, so the check here is changed slightly.

    We will probably want to reconfigure the elevated privilege to be a separate thing from configuring
    a terminal, but on Windows this will be a bit more complex and will better be done as a separate
    piece of work.

Description

reviewpad:summary

github-advanced-security[bot]
github-advanced-security bot found potential problems May 16, 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: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
@jacderida
feat: provide --owner arg for add cmd
7d8517b 
The `--owner` argument on the `add` command will be passed to the node service definition. It is
also retained on upgrade. Test cases were added for each.
@jacderida
chore: reconfigure local network owner args
514760b 
The `local run` and `local join` commands had an optional `--owner` argument being used in testing
scenarios to pass `--owner` to each `safenode` process. In its absence, the application code was
checking if it was running in a CI context, and if so, it was assigning an individual owner to each
node. This owner would be in the form "node_X", where "X" was replaced with the node number. If the
code determined it wasn't running in CI, it would still launch the node with `--owner`, assigning a
default value.

This behaviour has been changed such that `safenode` will only be passed `--owner` if it has also
been used on the node manager `local run/join` commands. This lets `safenode` itself provide a
default value. Now, the `local run --owner` argument assigns the same owner to each `safenode`
process in the local network, and a new argument, `--owner-prefix`, assigns an individual owner to
each node, based on a prefix. For example, if you use `--owner-prefix node`, the nodes will be
assigned owners "node_1", "node_2", and so on.

We need to configure the CI system to specify owners where it needs to. This new setup prevents CI
details leaking in to the application code.
@jacderida jacderida force-pushed the node_man-feat-add_cmd_owner_arg branch from 80c41da to 818afc9 Compare May 17, 2024 16:53
github-advanced-security[bot]
github-advanced-security bot found potential problems May 17, 2024
View reviewed changes
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
@jacderida jacderida force-pushed the node_man-feat-add_cmd_owner_arg branch from 818afc9 to 9452127 Compare May 17, 2024 17:50
@jacderida
ci: use owners on memcheck workflow local network
64638f0 
With the new `--owner-prefix` argument available on the `local run` command and the local testnet
action updated for this, we can use it in the `memcheck` workflow to specify that nodes should be
assigned the owners "node_1", "node_2", and so on.
@jacderida
feat: supply discord username on launchpad
2222787 
The Discord username is hooked up to the `safenode` `--owner` argument in the service definition.
@jacderida jacderida force-pushed the node_man-feat-add_cmd_owner_arg branch from 9452127 to e5ceb19 Compare May 17, 2024 21:38
@jacderida jacderida changed the title WIP -- feat: provide --owner arg for add cmd feat: provide --owner arg for add cmd May 17, 2024
@jacderida
chore: check we are in terminal before creating one
cf3a4a4 
I tried to run `node-launchpad` in a headless VM and it said it couldn't find a terminal. We don't
need to find a terminal if we are already running in one, so the check here is changed slightly.

We will probably want to reconfigure the elevated privilege to be a separate thing from configuring
a terminal, but on Windows this will be a bit more complex and will better be done as a separate
piece of work.
@jacderida jacderida force-pushed the node_man-feat-add_cmd_owner_arg branch from e5ceb19 to cf3a4a4 Compare May 17, 2024 21:43
@joshuef joshuef added this pull request to the merge queue May 19, 2024
Merged via the queue into maidsafe:main with commit db8be2d May 19, 2024
34 checks passed
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.

None yet
2 participants
@jacderida @joshuef