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

Update seednode/pricenode install scripts #4319

Merged
merged 4 commits into from
Jun 25, 2020

Conversation

devinbileck
Copy link
Member

While deploying a new seednode instance on Ubuntu 20.04, I ran into a few issues with the install script and README. This PR addresses those issues.

The install_collectd_debian.sh script reads user input to obtain the
onion address. However, when you pipe the output of curl into the shell
you're making the script text be standard input of the shell, which
takes it in as commands to run. After that, there's nothing left to
read. Even if it were to try, it wouldn't get anything from the terminal
input, because it's not connected to it. The pipe has replaced standard
input for the shell process.

Instead, create a pipe for bash to read the output of curl from and
provide it as the script file argument. In this case, the standard input
of the script is still connected to the terminal, and read will work.
@devinbileck devinbileck requested a review from cbeams as a code owner June 18, 2020 05:40
Copy link
Contributor

@cbeams cbeams left a comment

Choose a reason for hiding this comment

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

utACK. Thanks, Devin!

Before merging, I'd recommend at least one of the other @bisq-network/pricenode-operators try these changes out for themselves.

@ripcurlx
Copy link
Contributor

@wiz Could you have a look at this changes before I merge this based on #4319 (review)

@wiz
Copy link
Contributor

wiz commented Jun 25, 2020

utACK
Devin, thanks for fixing this!

@ripcurlx ripcurlx merged commit ab92350 into bisq-network:master Jun 25, 2020
@devinbileck devinbileck deleted the update-install-scripts branch July 6, 2020 06:28
@ripcurlx ripcurlx added this to the v1.3.6 milestone Jul 6, 2020
@@ -34,6 +34,11 @@ echo "[*] Upgrading apt packages"
sudo -H -i -u "${ROOT_USER}" DEBIAN_FRONTEND=noninteractive apt-get update -q
sudo -H -i -u "${ROOT_USER}" DEBIAN_FRONTEND=noninteractive apt-get upgrade -qq -y

echo "[*] Installing Git LFS"
sudo -H -i -u "${ROOT_USER}" curl -s https://packagecloud.io/install/repositories/github/git-lfs/script.deb.sh | bash
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed? Doesn't ubuntu have a git-lfs package by default?

Copy link
Contributor

Choose a reason for hiding this comment

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

I also think apt install git-lfs should work just fine. No need for external packages.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is used to ensure the latest git-lfs package is installed. I don't recall now, but perhaps the latest isn't necessary for our needs and it should be fine to remove this line.
https://github.com/git-lfs/git-lfs/wiki/Installation#debian-and-ubuntu

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.

5 participants