Refactor args to drop -- prefix, be more configurable

ingest/ledgerbackend/captive_core_backend.go

@@ -261,7 +261,7 @@ func (c *CaptiveStellarCore) openOnlineReplaySubprocess(ctx context.Context, fro
 
 	runFrom, ledgerHash, err := c.runFromParams(ctx, from)
 	if err != nil {
-		return errors.Wrap(err, "error calculating ledger and hash for stelar-core run")
+		return errors.Wrap(err, "error calculating ledger and hash for stellar-core run")
 	}
 
 	err = c.stellarCoreRunner.runFrom(runFrom, ledgerHash)

ingest/ledgerbackend/stellar_core_runner.go

@@ -113,7 +113,7 @@ func newStellarCoreRunner(config CaptiveCoreConfig, mode stellarCoreRunnerMode)
 		return nil, errors.New(fmt.Sprintf("%s is not a directory", fullStoragePath))
 	} else if err != nil {
 		return nil, errors.Wrap(err, fmt.Sprintf(
-			"error accessing storage directory: %s", fullStoragePath))
+			"error accessing storage directory (%s)", fullStoragePath))
 	}
 
 	ctx, cancel := context.WithCancel(config.Context)

services/horizon/internal/integration/parameters_test.go

@@ -8,7 +8,9 @@ import (
 	"path"
 	"strings"
 	"testing"
+	"time"
 
+	horizon "github.com/stellar/go/services/horizon/internal"
 	"github.com/stellar/go/services/horizon/internal/test/integration"
 
 	"github.com/stretchr/testify/assert"
@@ -17,16 +19,16 @@ import (
 
 func NewParameterTest(t *testing.T, params map[string]string) *integration.Test {
 	config := integration.Config{
-		ProtocolVersion:   17,
-		DontStartHorizon:  true,
-		HorizonParameters: params,
+		ProtocolVersion:    17,
+		DontStartHorizon:   true,
+		HorizonParameters:  params,
+		HorizonEnvironment: map[string]string{}, // TODO: test these too
 	}
 	return integration.NewTest(t, config)
 }
 
 func TestHorizonWorksWithoutCaptiveCore(t *testing.T) {
-	// This is a regression test, sourced from
-	// https://github.com/stellar/go/issues/3507
+	// This is a regression test sourced from https://github.com/stellar/go/issues/3507
 	test := NewParameterTest(t, map[string]string{
 		"--enable-captive-core-ingestion": "false",
 		"--ingest":                        "true",
@@ -42,30 +44,45 @@ func TestFatalScenarios(t *testing.T) {
 }
 
 // Ensures that BUCKET_DIR_PATH is not an allowed value for Captive Core.
+const (
+	BUCKET_DIR_DISALLOWED_TOML = `
+		PEER_PORT=11725
+		ARTIFICIALLY_ACCELERATE_TIME_FOR_TESTING=true
+
+		UNSAFE_QUORUM=true
+		FAILURE_SAFETY=0
+		BUCKET_DIR_PATH="/tmp"
+
+		[[VALIDATORS]]
+		NAME="local_core"
+		HOME_DOMAIN="core.local"
+		# From SACJC372QBSSKJYTV5A7LWT4NXWHTQO6GHG4QDAVC2XDPX6CNNXFZ4JK
+		PUBLIC_KEY="GD5KD2KEZJIGTC63IGW6UMUSMVUVG5IHG64HUTFWCHVZH2N2IBOQN7PS"
+		ADDRESS="localhost"
+		QUALITY="MEDIUM"`
+	CAPTIVE_CORE_CONFIG_STATE_TOML = `
+		PEER_PORT=11725
+		ARTIFICIALLY_ACCELERATE_TIME_FOR_TESTING=true
+
+		UNSAFE_QUORUM=true
+		FAILURE_SAFETY=0
+
+		[[VALIDATORS]]
+		NAME="local_core"
+		HOME_DOMAIN="core.local"
+		PUBLIC_KEY="GD5KD2KEZJIGTC63IGW6UMUSMVUVG5IHG64HUTFWCHVZH2N2IBOQN7PS"
+		ADDRESS="localhost"
+		QUALITY="MEDIUM"`
+)
+
 func (suite *FatalTestCase) TestBucketDirDisallowed() {
 	defer createCaptiveCoreConfig(
-		"./captive-core.toml", `
-PEER_PORT=11725
-ARTIFICIALLY_ACCELERATE_TIME_FOR_TESTING=true
-
-UNSAFE_QUORUM=true
-FAILURE_SAFETY=0
-BUCKET_DIR_PATH="/tmp"
-
-[[VALIDATORS]]
-NAME="local_core"
-HOME_DOMAIN="core.local"
-# From SACJC372QBSSKJYTV5A7LWT4NXWHTQO6GHG4QDAVC2XDPX6CNNXFZ4JK
-PUBLIC_KEY="GD5KD2KEZJIGTC63IGW6UMUSMVUVG5IHG64HUTFWCHVZH2N2IBOQN7PS"
-ADDRESS="localhost"
-QUALITY="MEDIUM"`)()
+		"./captive-core.toml", BUCKET_DIR_DISALLOWED_TOML)()
 
 	const STORAGE_PATH string = "./test_no-bucket-dir"
-
 	test := NewParameterTest(suite.T(), map[string]string{
-		"--captive-core-storage-path": STORAGE_PATH,
-		"--stellar-core-binary-path":  "/usr/local/bin/stellar-core",
-		"--captive-core-config-path":  "./captive-core.toml",
+		"--captive-core-storage-path":     STORAGE_PATH,
+		horizon.CaptiveCoreConfigPathName: "./captive-core.toml",
 	})
 	defer os.RemoveAll(STORAGE_PATH)
 
@@ -75,35 +92,24 @@ QUALITY="MEDIUM"`)()
 }
 
 // Ensures that the filesystem ends up in the correct state with Captive Core.
-func (suite *FatalTestCase) TestCaptiveCoreConfigFilesystemState() {
-	t := suite.T()
-
+func TestCaptiveCoreConfigFilesystemState(t *testing.T) {
 	defer createCaptiveCoreConfig(
-		"./captive-core.toml", `
-PEER_PORT=11725
-ARTIFICIALLY_ACCELERATE_TIME_FOR_TESTING=true
-
-UNSAFE_QUORUM=true
-FAILURE_SAFETY=0
-
-[[VALIDATORS]]
-NAME="local_core"
-HOME_DOMAIN="core.local"
-PUBLIC_KEY="GD5KD2KEZJIGTC63IGW6UMUSMVUVG5IHG64HUTFWCHVZH2N2IBOQN7PS"
-ADDRESS="localhost"
-QUALITY="MEDIUM"`)()
+		"./captive-core.toml", CAPTIVE_CORE_CONFIG_STATE_TOML)()
 
 	const STORAGE_PATH string = "./test_captive-core-works"
-
-	test := NewParameterTest(suite.T(), map[string]string{
-		"--captive-core-storage-path": STORAGE_PATH,
-		"--stellar-core-binary-path":  "/usr/local/bin/stellar-core",
-		"--captive-core-config-path":  "./captive-core.toml",
+	test := NewParameterTest(t, map[string]string{
+		"captive-core-storage-path":       STORAGE_PATH,
+		"captive-core-reuse-storage-dir":  "true",
+		horizon.CaptiveCoreConfigPathName: "./captive-core.toml",
+		horizon.StellarCoreURLFlagName:    "",
+		horizon.StellarCoreDBURLFlagName:  "",
 	})
 	defer os.RemoveAll(STORAGE_PATH)
 
 	err := test.StartHorizon()
 	assert.NoError(t, err)
+	// FIXME: IntegrationTest needs a big refactor so we can
+	time.Sleep(10 * time.Second)
 
 	runParameterMatrix(test, []ValidatorFunc{
 		func() { validateCaptiveCoreDiskState(test, STORAGE_PATH) },

services/horizon/internal/test/integration/integration.go

@@ -10,6 +10,7 @@ import (
 	"os/signal"
 	"path/filepath"
 	"strconv"
+	"strings"
 	"sync"
 	"syscall"
 	"testing"
@@ -49,9 +50,16 @@ type Config struct {
 	DontStartHorizon bool
 
 	// If you want to override the default parameters passed to Horizon, you can
-	// set this map accordingly. All of them are passed along as k=v, but if you
-	// pass an empty value, the parameter will be dropped.
-	HorizonParameters map[string]string
+	// set this map accordingly. All of them are passed along as --k=v, but if
+	// you pass an empty value, the parameter will be dropped. (Note that you
+	// should exclude the prepending `--` from keys; this is for compatibility
+	// with the constant names in flags.go)
+	//
+	// You can also control the environmental variables in a similar way, but
+	// note that CLI args take precedence over envvars, so set the corresponding
+	// CLI arg empty.
+	HorizonParameters  map[string]string
+	HorizonEnvironment map[string]string
 }
 
 type CaptiveConfig struct {
@@ -109,15 +117,15 @@ func NewTest(t *testing.T, config Config) *Test {
 
 	// We either test Captive Core through environment variables, or through
 	// custom Horizon parameters.
+	i.coreConfig.binaryPath = os.Getenv("CAPTIVE_CORE_BIN")
 	if os.Getenv("HORIZON_INTEGRATION_ENABLE_CAPTIVE_CORE") != "" {
-		i.coreConfig.binaryPath = os.Getenv("CAPTIVE_CORE_BIN")
 		i.coreConfig.configPath = filepath.Join(composePath, "captive-core-integration-tests.cfg")
 	}
 
-	if value, ok := config.HorizonParameters["--stellar-core-binary-path"]; ok {
+	if value, ok := config.HorizonParameters[horizon.StellarCoreBinaryPathName]; ok {
 		i.coreConfig.binaryPath = value
 	}
-	if value, ok := config.HorizonParameters["--captive-core-config-path"]; ok {
+	if value, ok := config.HorizonParameters[horizon.CaptiveCoreConfigPathName]; ok {
 		i.coreConfig.configPath = value
 	}
 
@@ -131,7 +139,6 @@ Config.HorizonParameters with the appropriate parameter(s).`)
 	i.runComposeCommand("up", "--detach", "--quiet-pull", "--no-color", "core")
 	i.cclient = &stellarcore.Client{URL: "http://localhost:" + strconv.Itoa(stellarCorePort)}
 	i.waitForCore()
-
 	i.prepareShutdownHandlers()
 
 	if !config.DontStartHorizon {
@@ -251,30 +258,28 @@ func (i *Test) StartHorizon() error {
 	coreBinaryPath := i.coreConfig.binaryPath
 	captiveCoreConfigPath := i.coreConfig.configPath
 
-	i.t.Logf("config: %+v", i.coreConfig)
-
 	var DEFAULT_ARGS = map[string]string{
-		"--stellar-core-url": i.cclient.URL,
-		"--stellar-core-db-url": fmt.Sprintf(
+		"stellar-core-url": i.cclient.URL,
+		"stellar-core-db-url": fmt.Sprintf(
 			"postgres://postgres:%s@%s:%d/stellar?sslmode=disable",
 			stellarCorePostgresPassword,
 			hostname,
 			stellarCorePostgresPort,
 		),
-		"--stellar-core-binary-path":      coreBinaryPath,
-		"--captive-core-config-path":      captiveCoreConfigPath,
-		"--captive-core-http-port":        "21626",
-		"--enable-captive-core-ingestion": strconv.FormatBool(len(coreBinaryPath) > 0),
-		"--ingest":                        "true",
-		"--history-archive-urls":          fmt.Sprintf("http://%s:%d", hostname, historyArchivePort),
-		"--db-url":                        horizonPostgresURL,
-		"--network-passphrase":            i.passPhrase,
-		"--apply-migrations":              "true",
-		"--admin-port":                    strconv.Itoa(i.AdminPort()),
-		"--port":                          "8000",
+		"stellar-core-binary-path":      coreBinaryPath,
+		"captive-core-config-path":      captiveCoreConfigPath,
+		"captive-core-http-port":        "21626",
+		"enable-captive-core-ingestion": strconv.FormatBool(len(coreBinaryPath) > 0),
+		"ingest":                        "true",
+		"history-archive-urls":          fmt.Sprintf("http://%s:%d", hostname, historyArchivePort),
+		"db-url":                        horizonPostgresURL,
+		"network-passphrase":            i.passPhrase,
+		"apply-migrations":              "true",
+		"admin-port":                    strconv.Itoa(i.AdminPort()),
+		"port":                          "8000",
 		// due to ARTIFICIALLY_ACCELERATE_TIME_FOR_TESTING
-		"--checkpoint-frequency": "8",
-		"--per-hour-rate-limit":  "0", // disable rate limiting
+		"checkpoint-frequency": "8",
+		"per-hour-rate-limit":  "0", // disable rate limiting
 	}
 
 	merged := mergeMaps(DEFAULT_ARGS, i.config.HorizonParameters)
@@ -283,16 +288,28 @@ func (i *Test) StartHorizon() error {
 	// empty values means drop the parameter.
 	args := make([]string, 0, len(merged))
 	for key, value := range merged {
-		if value != "" {
-			args = append(args, fmt.Sprintf("%s=%s", key, value))
+		if value == "" {
+			continue
 		}
+
+		args = append(args, fmt.Sprintf("--%s=%s", key, value))
 	}
 
 	// initialize core arguments
 	i.t.Log("Horizon command line:", args)
+	var env strings.Builder
+	for key, value := range i.config.HorizonEnvironment {
+		env.WriteString(fmt.Sprintf("%s=%s", key, value))
+	}
+	i.t.Logf("Horizon environmental variables: %s\n", env.String())
+
+	// prepare env
 	cmd.SetArgs(args)
-	var err error
+	for key, value := range i.config.HorizonEnvironment {
+		os.Setenv(key, value)
+	}
 
+	var err error
 	if err = configOpts.Init(cmd); err != nil {
 		return errors.Wrap(err, "cannot initialize params")
 	}
@@ -316,6 +333,7 @@ func (i *Test) StartHorizon() error {
 		close(done)
 	}()
 	i.appStopped = done
+
 	return nil
 }