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: uniform behaviour for all add commands #1455

Merged
merged 3 commits into from
Mar 19, 2024

Conversation

jacderida
Copy link
Contributor

@jacderida jacderida commented Mar 15, 2024

  • f9998cc feat: uniform behaviour for all add commands

    The three add commands for safenode, safenodemand and faucet now behave in a uniform way,
    all supporting --path, --url and --version arguments. These enable a binary to be provided via
    a path, a URL pointing to a tar/zip containing the binary, or by a specific version number,
    respectively.

    The safenodemand service also now supports running as a non-root user.

  • 53e2bbd chore: upgrade sn-releases to new minor version

    The new version of sn-releases uses a Version type rather than strings, and also renamed a
    trait, so the code had to be updated accordingly.

  • 61a4245 chore: run safenodemand service as root

    Creating a user for running the safenodemand service doesn't make as much sense because the user
    must have root access. We can leave it up to the user to create an account and set it up for
    passwordless sudo, then provide an optional --user argument. For now we don't have this, and just
    go with root.

Description

reviewpad:summary

.with(eq(ServiceInstallCtx {
args: vec![
OsString::from("--rpc"),
OsString::from("127.0.0.1:15002"),

Check notice

Code scanning / devskim

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

Do not leave debug code in production
.with(eq(ServiceInstallCtx {
args: vec![
OsString::from("--rpc"),
OsString::from("127.0.0.1:15001"),

Check notice

Code scanning / devskim

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

Do not leave debug code in production
.with(eq(ServiceInstallCtx {
args: vec![
OsString::from("--rpc"),
OsString::from("127.0.0.1:15000"),

Check notice

Code scanning / devskim

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

Do not leave debug code in production
@JasonPaulGithub
Copy link
Contributor

Marked do not merge but we have approvals, hope thats ok @RolandSherwin

@JasonPaulGithub JasonPaulGithub added the Medium Medium sized PR label Mar 18, 2024
@RolandSherwin
Copy link
Member

Hey @JasonPaulGithub, this PR depends on a change from sn-releases to land, so we cannot get this in until then.

@JasonPaulGithub
Copy link
Contributor

Hey @JasonPaulGithub, this PR depends on a change from sn-releases to land, so we cannot get this in until then.

@RolandSherwin! 👍 👍

@JasonPaulGithub JasonPaulGithub added the Pending on Upcoming Changes Branch is waiting on incoming changes (do not merge) label Mar 18, 2024
The three `add` commands for `safenode`, `safenodemand` and `faucet` now behave in a uniform way,
all supporting `--path`, `--url` and `--version` arguments. These enable a binary to be provided via
a path, a URL pointing to a tar/zip containing the binary, or by a specific version number,
respectively.

The `safenodemand` service also now supports running as a non-root user.
The new version of `sn-releases` uses a `Version` type rather than strings, and also renamed a
trait, so the code had to be updated accordingly.
@jacderida jacderida removed DoNotMerge Pending on Upcoming Changes Branch is waiting on incoming changes (do not merge) labels Mar 19, 2024
Creating a user for running the `safenodemand` service doesn't make as much sense because the user
must have root access. We can leave it up to the user to create an account and set it up for
passwordless sudo, then provide an optional `--user` argument. For now we don't have this, and just
go with root.
@jacderida jacderida merged commit 329d326 into maidsafe:main Mar 19, 2024
56 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Medium Medium sized PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants