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

Node Manager: A Few Minor UX Enhancements #1651

Merged
merged 6 commits into from
May 1, 2024

Conversation

jacderida
Copy link
Contributor

@jacderida jacderida commented Apr 26, 2024

  • e247ef7 fix: apply interval only to non-running nodes

    The node manager start command has an --interval argument that can be used as a time-based delay
    between starting nodes. The problem is, the interval would apply to nodes that were already running,
    which is a waste of time. Now the interval is not applied to those nodes.

  • be04805 chore: use better banners

    These new banners for the node manager are a bit more eye catching, and also fix the incorrect sizes
    of the previous banners in the status --details command.

  • 8a0b296 chore: output reward balance in status --json cmd

    This was a community feedback request.

    The reward balance is now output as part of the status --json command, which involved adding the
    field to the information tracked by the registry.

    BREAKING CHANGE: previously written registry files will not have this new field and so won't
    deserialize correctly.

  • 5a32365 tests: use node registry for status

    Rather than parsing the text-based output of the status command, we now switch to using status --json, which we can then serialize into the NodeRegistry, and any status can be obtained from
    there.

    This prevents breakage when the text output changes.

  • 5e801a0 fix: change reward balance to optional

    Due to it being possible to run the node manager status command before any services have been
    started, the reward balance should be an optional value, since the node's wallet has not been
    created before it starts.

  • 94ef20e test: disable node man integration tests

    These tests are no longer passing and it's not completely clear why. In any case, I think they need
    to be a bit more isolated, and operate on their own local network.

    At the moment there is not enough time to address that.

Description

reviewpad:summary

The node manager `start` command has an `--interval` argument that can be used as a time-based delay
between starting nodes. The problem is, the interval would apply to nodes that were already running,
which is a waste of time. Now the interval is not applied to those nodes.
These new banners for the node manager are a bit more eye catching, and also fix the incorrect sizes
of the previous banners in the `status --details` command.
This was a community feedback request.

The reward balance is now output as part of the `status --json` command, which involved adding the
field to the information tracked by the registry.

BREAKING CHANGE: previously written registry files will not have this new field and so won't
deserialize correctly.
@jacderida jacderida force-pushed the node_man-minor_ux_enhancements branch 2 times, most recently from d61c98e to 1f8c456 Compare April 30, 2024 16:28
Rather than parsing the text-based output of the `status` command, we now switch to using `status
--json`, which we can then serialize into `StatusSummary`, and any status can be obtained from
there.

This prevents breakage when the text output changes.
Due to it being possible to run the node manager status command before any services have been
started, the reward balance should be an optional value, since the node's wallet has not been
created before it starts.
@jacderida jacderida force-pushed the node_man-minor_ux_enhancements branch from f49a156 to 5e801a0 Compare April 30, 2024 18:19
These tests are no longer passing and it's not completely clear why. In any case, I think they need
to be a bit more isolated, and operate on their own local network.

At the moment there is not enough time to address that.
@jacderida jacderida merged commit 79ea950 into maidsafe:main May 1, 2024
34 checks passed
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.

2 participants