Skip to content
This repository has been archived by the owner on Feb 1, 2024. It is now read-only.

Feature/automate install script #667

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

BenRacicot
Copy link

This is the installation script for MacOS written by myself and Nikhil to automate some or all of Kelp's initial setup.

Copy link
Contributor

@nikhilsaraf nikhilsaraf left a comment

Choose a reason for hiding this comment

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

Added some comments. We should be good to merge after these changes. This is going to be amazing!

@@ -131,7 +133,9 @@ To compile Kelp from source:
* `./scripts/build.sh`
8. Confirm one new binary file exists with version information.
* `./bin/kelp version`
9. Set up CCXT to use an expanded set of priceFeeds and orderbooks (see the [Using CCXT](#using-ccxt) section for details)
9. Run the GUI
Copy link
Contributor

Choose a reason for hiding this comment

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

let's swap step 9 and 10. we should run ccxt-rest before we run the GUI

To compile Kelp from source:
_Note for MacOS Users: Running [install-macos.sh][install-macos-script] should automate the steps below. (including cloning the Kelp repo to the correct Go folder) Remember, manual installation of PostgreSQL and Docker are additionally required._

## Manual Installation Steps
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a section called "Automated Install for MacOS" above this section on "Manual Installation Steps"?
Then you can include the text you have in the note above as part of that section. That will give more importance to your automated script.

# shellcheck disable=SC2016
set -e

# Set this machine's dependencies status(es) with the OK flag
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add to this comment that this variable is what we use as the return value from functions. we do so by updating the value of the OK variable before we return from the function and the calling code can read the OK variable.

ARCH="$(uname -m)"

case $OS in
"Linux")
Copy link
Contributor

Choose a reason for hiding this comment

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

will this run on Linux too?

esac

if [ -z "$PLATFORM" ]; then
echo "Your operating system is not supported by the script."
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add the name of the OS that is unsupported?

echo "Your operating system '$OS' is not supported by the script."


function postGoInstall() {
cloneIntoDir
isGlide
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to invoke installGlide here if isGlide is false?

#**********************
# Execute the functions
#**********************
cd
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a comment here why we are doing this cd -- IIRC it was to get to a deterministic starting working directory

cd

isNode
if [ $OK ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we can call checkDeps here. so it executes all of these checks and we don't need this duplicate code.
then we can reuse the $OK variable and if it is false we can simply to an exit 1.

Example:

#checkDeps will set the OK variable to false if at least 1 dependency is missing
checkDeps
if [ $OK ]; then
    # do nothing
else
    exit 1
fi

Since we don't want to exit in the case where Golang does not exist, we can inline the go check logic from the checkDeps function into postGoInstall() right after we call checkDeps (line 190)

if $OK; then
echo "`go version` is installed"
else
echo "Goland is not installed"
Copy link
Contributor

Choose a reason for hiding this comment

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

this should say Golang instead of Goland -- Goland is a Go-specific IDE so this can be confusing


isGo
if $OK; then
echo "true for some reason"
Copy link
Contributor

Choose a reason for hiding this comment

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

this echo statement seems confusing. we don't need to echo anything here since we echo the go version below.

@nikhilsaraf
Copy link
Contributor

@BenRacicot based on keybase conversation we can merge this as-is and I can make the suggested changes after this PR is merged. Can you please sign the CLA so we can accept this commit? thanks for the submission! :)
https://forms.gle/9FBgjDnNYv1abnKD7

# pwd
mkdir -p $GOPATH/github.com/stellar/kelp
echo "Cloning Kelp into $GOPATH/src/github.com/stellar/kelp"
git clone https://github.com/stellar/kelp.git $GOPATH/src/github.com/stellar/kelp

Choose a reason for hiding this comment

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

I think that /src needs to be removed since in the line 156 is not included in the path.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants