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

Fold stellar/horizon into stellar/go/services/horizon #93

Merged
merged 17 commits into from
Oct 6, 2017

Conversation

nikhilsaraf
Copy link
Contributor

@nikhilsaraf nikhilsaraf commented Oct 4, 2017

Notes for the current commit:

  1. stellar/go/services/horizon/scripts/run_tests.bash passes, and the horizon server builds and serves on localhost:8000

  2. I've updated stellar/go/services/horizon/README.md to link to the stellar/go/releases page so these download links are currently broken. We should backfill these links by creating new releases, or update these links to point to the old files in stellar/horizon/releases until we decide to push out a new horizon release.

  3. The horizon docs now point to filing all issues to the “go” monorepo (stellar/go/issues) so we should expect that change in behavior from the community down the road.

  4. The only bash script I've updated is stellar/go/services/horizon/scripts/run_tests.bash; all other scripts in stellar/go/services/horizon/scripts are currently broken because they depend on gb. We should work on fixing these scripts as a separate commit if we can tolerate having them broken for a short window — I can work on fixing them after this commit.

  5. I will also need to port any changes made to horizon that were made after I started my changes, there's 2 so far I think — I can include these changes after this commit is approved.

Changes needed in other repos to support this move:

  1. Need to update https://github.com/stellar/developers/blob/master/repos.json to point to the new url — I will work on this after this commit. (UPDATE: see updated developers repo to reflect the change in horizon's location developers#88)

  2. I need to add a deprecated message to the old HORIZON repo’s README.md with a pointer to the new location — I can work on this after this commit. (UPDATE: see add repo moved message to stellar/horizon repo stellar-deprecated/horizon#407)

  3. UPDATE: stellar/docs needs to be updated with the new url (see make "Software" tab in docs point to the new horizon location stellar-deprecated/docs#233)

@nikhilsaraf
Copy link
Contributor Author

had not included the /tree/master portion for web-based github urls so fixed that in commit no. 4

@nullstyle
Copy link
Contributor

Looks like we're getting failures in Travis when running the test suite.

…x test failures in goji.io/mux ("context" import)
@nikhilsaraf
Copy link
Contributor Author

fixed a dependency issue (in commit 5) by picking the older option between stellar/go and stellar/horizon for goji.io, will address the remaining test failures to resolve the build.

…o prevent test failures caused by db parallelism
did not add the following lines (build_artifacts.bash is broken right now, let's see if it works without that):
before_deploy:
  - bash scripts/build_artifacts.bash

cache:
  directories:
  - vendor/src
  - pkg
@@ -0,0 +1,2 @@
export PATH=$PWD/bin:$PATH
Copy link
Contributor

Choose a reason for hiding this comment

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

This file is no longer needed since we aren't using gb anymore

.travis.yml Outdated
go:
- '1.6'
- '1.7'
- '1.8'
- tip
env:
- GO15VENDOREXPERIMENT="1"
global:
- GO15VENDOREXPERIMENT="1"
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need this anymore... go's vendor functionality is standard now on all of our supported versions (1.7, 1.8, 1.9)

.travis.yml Outdated
- GO15VENDOREXPERIMENT="1"
global:
- GO15VENDOREXPERIMENT="1"
- DATABASE_URL=postgres://localhost:5432/horizon_test?sslmode=disable&user=postgres
Copy link
Contributor

Choose a reason for hiding this comment

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

These environment variables (DATABASE_URL and STELLAR_CORE_DATABASE_URL) should not be needed to run our tests in the monorepo. Before we merge I think we should make the necessary changes to excise the need for their existence from the test suite.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed it in the latest commit, hopefully the build passes with that change.

install:
- wget https://github.com/Masterminds/glide/releases/download/v0.12.3/glide-v0.12.3-linux-amd64.tar.gz
- tar -xzvf glide-v0.12.3-linux-amd64.tar.gz
- cp linux-amd64/glide $GOPATH/bin
before_script:
- psql -c 'create database "horizon_test";' -U postgres
Copy link
Contributor

Choose a reason for hiding this comment

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

likewise to the environment variables I think we should remove the static test databases from horizon's test suite before we merge. If possible, switch to the dbtest.Postgres() system we talked about.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as we discussed, I'll include this change in a separate commit once this is merged, will be easier to track the changes for the db-parallelization.

@@ -1,7 +1,7 @@
#! /bin/bash
set -e

TESTS=$(glide novendor | sed 's/^\./github.com\/stellar\/go/g')
TESTS=$(glide novendor | grep "./services/" | sed 's/^\./github.com\/stellar\/go/g')
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe this is correct: This script should be testing everything in the monorepo but horizon. The command you've added here encodes "only run the services tests", right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, this is incorrect, it should have the -v flag. I added that as a fix in the commit after that.

@nikhilsaraf
Copy link
Contributor Author

Looks like the build is passing. This leaves the following changes which I plan to make once this PR is merged to fully complete the changes needed in this repo:

  1. clean up the bash scripts in services/horizon/scripts -- this will unblock deployments for horizon since build_artifacts.bash is one of those scripts
  2. port any changes that were made since I started my port (1 change)
  3. convert tests to use the Postgres() call instead of the Start() call to parallelize the horizon tests and use the existing test script in .travis.yml

These are a lot of commits. Do you want me to clean them up into a fewer number of commits (or one commit) and re-submit with this finalized version?
@nullstyle

@@ -11,18 +11,15 @@ func TestLedgerCache(t *testing.T) {
defer tt.Finish()
q := &Q{tt.HorizonSession()}

t.Run("queue and load", func(t *testing.T) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was failing in the recent run because it could not recognize t.Run (it should have).
I did not look into fixing it because this is the only case where we use t.Run and it seemed redundant for this test case
please correct me if that's wrong @nullstyle

@@ -1,7 +1,7 @@
#! /bin/bash
set -e

TESTS=$(glide novendor | sed 's/^\./github.com\/stellar\/go/g')
TESTS=$(glide novendor | grep "./services/" | sed 's/^\./github.com\/stellar\/go/g')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, this is incorrect, it should have the -v flag. I added that as a fix in the commit after that.

.travis.yml Outdated
- GO15VENDOREXPERIMENT="1"
global:
- GO15VENDOREXPERIMENT="1"
- DATABASE_URL=postgres://localhost:5432/horizon_test?sslmode=disable&user=postgres
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed it in the latest commit, hopefully the build passes with that change.

install:
- wget https://github.com/Masterminds/glide/releases/download/v0.12.3/glide-v0.12.3-linux-amd64.tar.gz
- tar -xzvf glide-v0.12.3-linux-amd64.tar.gz
- cp linux-amd64/glide $GOPATH/bin
before_script:
- psql -c 'create database "horizon_test";' -U postgres
Copy link
Contributor Author

Choose a reason for hiding this comment

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

as we discussed, I'll include this change in a separate commit once this is merged, will be easier to track the changes for the db-parallelization.

Copy link
Contributor Author

@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.

finished the last bit of changes for this repo by fixing the bash scripts

@@ -1,7 +1,7 @@
#! /usr/bin/env bash
set -e

DIST="dist"
DIST="$GOPATH/dist"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

tested these locally and they work (except dev.bash)

all these follow the same pattern:

  • add the $GOPATH
  • convert gb build [-flags] to go install [-flags] github.com/stellar/go/services/horizon/cmd/horizon
  • convert gb test X to go test X
  • convert gb generate X to go generate X

CORE_SQL=./src/github.com/stellar/go/services/horizon/test/scenarios/$SCENARIO-core.sql
HORIZON_SQL=./src/github.com/stellar/go/services/horizon/test/scenarios/$SCENARIO-horizon.sql
CORE_SQL=$GOPATH/src/github.com/stellar/go/services/horizon/test/scenarios/$SCENARIO-core.sql
HORIZON_SQL=$GOPATH/src/github.com/stellar/go/services/horizon/test/scenarios/$SCENARIO-horizon.sql
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, GOPATH isn't guaranteed to be single-valued, so we can't use it directly. You might look into using a script-relative path, the equivalent of ../../test/scenarios/$SCENARIO-core.sql.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, converted all scripts to use a script-relative path, and created a new variable $GOTOP in the scripts where it was needed

@@ -5,8 +5,6 @@ package db

import (
"log"
"os"
Copy link
Contributor

Choose a reason for hiding this comment

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

You might be interested in leveraging https://godoc.org/golang.org/x/tools/cmd/goimports to help avoid this in the future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, will look into integrating this into my IDE.

@nikhilsaraf
Copy link
Contributor Author

updated the scripts. let me know if there's anything else. I'm looking into any changes needed in the stellar/docs repo.

@nikhilsaraf nikhilsaraf merged commit 53155ad into stellar:master Oct 6, 2017
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.

None yet

2 participants