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

A0-3432: run_nodes.sh revamp, in particular run bootnode as RPC node #1472

Merged
merged 5 commits into from
Nov 6, 2023

Conversation

Marcin-Radecki
Copy link
Contributor

@Marcin-Radecki Marcin-Radecki commented Nov 2, 2023

Description

This PR revamps scripts/run_nodes.sh so that

  • there are only two categories of nodes: RPC node and validator node
  • boot node is always the first RPC node
  • number of either node type is configurable, but sum must not exceed 10,
  • script cmd line args was reworked which is a breaking change
  • by default, script ideas all the following: builds aleph-node, bootstraps chain, purges db, removes AlephBFT backups. User can opt-out from either of these, but obviously some of them must be present if others are etc, so the script checks all that
  • script now logs with timestamps and colors, which is justifies as the script is not intended to run in CI
  • as the script is a front card of our repo and most likely the first point of contact with the new user, it contains now a compact intro
    to chain bootstrap in general
  • jq is now needed, though is not hard blocker for me to revert to previous behaviour, now it is just cleaner in bash to parse output of aleph-node key inspect
  • base path default was changed from /tmp/ to local ./run-nodes-sh, this is because the user must have write permissions in the repo he or she just cloned, and it's better to have a base path that is not shared with any other files/dirs.
  • arguments were synced with docker/docker_entrypoint.sh; they are indeed duplicated now but it does not make sense to extract them to common .sh as at some point run_nodes.sh might become obsolete due to dockerization

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

scripts/run_nodes.sh Outdated Show resolved Hide resolved
scripts/run_nodes.sh Outdated Show resolved Hide resolved
scripts/run_nodes.sh Outdated Show resolved Hide resolved
scripts/run_nodes.sh Outdated Show resolved Hide resolved
Copy link
Contributor

@ggawryal ggawryal left a comment

Choose a reason for hiding this comment

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

Great job, I like the idea of keeping that script well-documented and user-friendly as a first contact point with the repo. Left minor comments.
Also, maybe @fixxxedpoint will know more context, there was some issue why we disabled purging backups by default (A0-2846). Nevertheless, I think we should be good unless user does something very strange.

scripts/run_nodes.sh Outdated Show resolved Hide resolved
scripts/run_nodes.sh Outdated Show resolved Hide resolved
scripts/run_nodes.sh Show resolved Hide resolved
scripts/run_nodes.sh Outdated Show resolved Hide resolved
scripts/run_nodes.sh Outdated Show resolved Hide resolved
scripts/run_nodes.sh Show resolved Hide resolved
scripts/run_nodes.sh Outdated Show resolved Hide resolved
functions. This does not have an affect in this script since iteration
over a string delimated by a space works the same.
Copy link
Contributor

@fixxxedpoint fixxxedpoint left a comment

Choose a reason for hiding this comment

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

Well done!

@Marcin-Radecki Marcin-Radecki added this pull request to the merge queue Nov 6, 2023
Merged via the queue into main with commit 21ead16 Nov 6, 2023
9 checks passed
@Marcin-Radecki Marcin-Radecki deleted the A0-3432 branch November 6, 2023 10:27
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.

3 participants