diff --git a/cmd/spinloop/alias_test.go b/cmd/spinloop/alias_test.go index 8546c0ca..05a5fbb1 100644 --- a/cmd/spinloop/alias_test.go +++ b/cmd/spinloop/alias_test.go @@ -832,7 +832,7 @@ func TestEnvAlias_ReachesRemote(t *testing.T) { t.Chdir(t.TempDir()) // no ./Spinloop, so only the variable can find it t.Setenv("SPINLOOP_ALIAS", "q3") - if err := cmdRemoteStop(nil); err != nil { + if err := cmdRemoteStop([]string{"--env", "default"}); err != nil { t.Fatalf("cmdRemoteStop with SPINLOOP_ALIAS: %v", err) } select { @@ -879,7 +879,7 @@ func TestEnvAlias_RemoteFailsRatherThanFallingBack(t *testing.T) { t.Chdir(t.TempDir()) t.Setenv("SPINLOOP_ALIAS", "nope") - err := cmdRemoteStop(nil) + err := cmdRemoteStop([]string{"--env", "default"}) if err == nil { t.Fatal("expected an error for an unregistered SPINLOOP_ALIAS") } @@ -903,7 +903,7 @@ func TestEnvAlias_RemoteFallsBackWithoutREMOTE(t *testing.T) { t.Chdir(t.TempDir()) t.Setenv("SPINLOOP_ALIAS", "q3") - err := cmdRemoteStop(nil) + err := cmdRemoteStop([]string{"--env", "default"}) if err == nil { t.Fatal("expected an error: there is no default endpoint config either") } diff --git a/cmd/spinloop/flagparse_test.go b/cmd/spinloop/flagparse_test.go index 1fe14b3b..b35be437 100644 --- a/cmd/spinloop/flagparse_test.go +++ b/cmd/spinloop/flagparse_test.go @@ -29,7 +29,7 @@ func TestPflagParseForms(t *testing.T) { "apply": func() error { return cmdApply([]string{"--nope"}) }, "serve": func() error { return cmdServe([]string{"--nope"}) }, "fleet metrics": func() error { return cmdFleet([]string{"metrics", "--nope"}) }, - "remote start": func() error { return cmdRemoteStart([]string{"--nope"}) }, + "remote start": func() error { return cmdRemoteStart([]string{"--env", "default", "--nope"}) }, "daemon": func() error { return cmdDaemon([]string{"--nope"}) }, } { if err := call(); err == nil || !strings.Contains(err.Error(), "unknown flag: --nope") { @@ -51,7 +51,7 @@ func TestPflagParseForms(t *testing.T) { // the proof that parsing continued past it. for name, call := range map[string]func() error{ "fleet metrics": func() error { return cmdFleet([]string{"metrics", "someNode", "--nope"}) }, - "remote env": func() error { return cmdRemoteEnv([]string{"somePath", "--nope"}) }, + "remote env": func() error { return cmdRemoteEnv([]string{"--env", "default", "somePath", "--nope"}) }, } { if err := call(); err == nil || !strings.Contains(err.Error(), "unknown flag: --nope") { t.Errorf("%s --nope = %v, want unknown-flag error", name, err) diff --git a/cmd/spinloop/last_active_test.go b/cmd/spinloop/last_active_test.go index 5838ff47..6328df27 100644 --- a/cmd/spinloop/last_active_test.go +++ b/cmd/spinloop/last_active_test.go @@ -48,7 +48,7 @@ func TestRemoteMetricsBarShowsLastActive(t *testing.T) { statsServer(t, runningWithActivity) out := captureStdout(t, func() { - if err := cmdRemoteMetrics([]string{"--format=bar"}); err != nil { + if err := cmdRemoteMetrics([]string{"--env", "default", "--format=bar"}); err != nil { t.Fatalf("cmdRemoteMetrics: %v", err) } }) @@ -69,7 +69,7 @@ func TestRemoteMetricsTableShowsLastActive(t *testing.T) { statsServer(t, runningWithActivity) out := captureStdout(t, func() { - if err := cmdRemoteMetrics([]string{"--format=table"}); err != nil { + if err := cmdRemoteMetrics([]string{"--env", "default", "--format=table"}); err != nil { t.Fatalf("cmdRemoteMetrics: %v", err) } }) @@ -86,7 +86,7 @@ func TestRemoteMetricsJSONCarriesLastActive(t *testing.T) { statsServer(t, runningWithActivity) out := captureStdout(t, func() { - if err := cmdRemoteMetrics([]string{"--format=json"}); err != nil { + if err := cmdRemoteMetrics([]string{"--env", "default", "--format=json"}); err != nil { t.Fatalf("cmdRemoteMetrics: %v", err) } }) @@ -124,7 +124,7 @@ func TestRemoteMetricsStoppedStillShowsLastActive(t *testing.T) { } { t.Run(format, func(t *testing.T) { out := captureStdout(t, func() { - if err := cmdRemoteMetrics([]string{"--format=" + format}); err != nil { + if err := cmdRemoteMetrics([]string{"--env", "default", "--format=" + format}); err != nil { t.Fatalf("cmdRemoteMetrics: %v", err) } }) @@ -154,7 +154,7 @@ func TestLastActiveZeroIdleStillRenders(t *testing.T) { } { t.Run(format, func(t *testing.T) { out := captureStdout(t, func() { - if err := cmdRemoteMetrics([]string{"--format=" + format}); err != nil { + if err := cmdRemoteMetrics([]string{"--env", "default", "--format=" + format}); err != nil { t.Fatalf("cmdRemoteMetrics: %v", err) } }) @@ -176,7 +176,7 @@ func TestLastActiveOmittedWithoutATimestamp(t *testing.T) { for _, format := range []string{"bar", "table"} { t.Run(format, func(t *testing.T) { out := captureStdout(t, func() { - if err := cmdRemoteMetrics([]string{"--format=" + format}); err != nil { + if err := cmdRemoteMetrics([]string{"--env", "default", "--format=" + format}); err != nil { t.Fatalf("cmdRemoteMetrics: %v", err) } }) @@ -215,7 +215,7 @@ func TestRemoteStatusShowsLastActive(t *testing.T) { }`) out := captureStdout(t, func() { - if err := cmdRemoteStatus(nil); err != nil { + if err := cmdRemoteStatus([]string{"--env", "default"}); err != nil { t.Fatalf("cmdRemoteStatus: %v", err) } }) @@ -234,7 +234,7 @@ func TestRemoteStatusZeroIdleStillRenders(t *testing.T) { statusServer(t, `{"state": "running", "healthy": true, "lastActiveAt": "2026-08-10T10:00:00Z"}`) out := captureStdout(t, func() { - if err := cmdRemoteStatus(nil); err != nil { + if err := cmdRemoteStatus([]string{"--env", "default"}); err != nil { t.Fatalf("cmdRemoteStatus: %v", err) } }) @@ -249,7 +249,7 @@ func TestRemoteStatusOmitsLastActiveWhenAbsent(t *testing.T) { statusServer(t, `{"state": "stopped", "healthy": false}`) out := captureStdout(t, func() { - if err := cmdRemoteStatus(nil); err != nil { + if err := cmdRemoteStatus([]string{"--env", "default"}); err != nil { t.Fatalf("cmdRemoteStatus: %v", err) } }) diff --git a/cmd/spinloop/main.go b/cmd/spinloop/main.go index 54afd28e..b5fdecf6 100644 --- a/cmd/spinloop/main.go +++ b/cmd/spinloop/main.go @@ -235,7 +235,7 @@ func applySelection(sel spinloop.Selection, h harness.Harness, spinloopPath, env } return err } - cfg, err := remote.LoadConfigFile(envPath, viperGetenv()) + cfg, err := remote.LoadEnvironment(envName, viperGetenv()) if err != nil { return err } @@ -1624,7 +1624,7 @@ func fetchRemoteEnv(sel spinloop.Selection, envName string, resolve func(string) } // The call crosses the network, and a cold control plane is not instant. fmt.Fprintf(os.Stderr, "Fetching the endpoint's environment from %s...\n", envName) - cfg, err := remote.LoadConfigFile(envPath, viperGetenv()) + cfg, err := remote.LoadEnvironment(envName, viperGetenv()) if err == nil { ctx, cancel := context.WithTimeout(context.Background(), remoteEnvTimeout) defer cancel() diff --git a/cmd/spinloop/remote.go b/cmd/spinloop/remote.go index b8500a5f..f0e6f36f 100644 --- a/cmd/spinloop/remote.go +++ b/cmd/spinloop/remote.go @@ -3,6 +3,7 @@ package main import ( "context" "encoding/json" + "errors" "fmt" "io" "maps" @@ -93,7 +94,7 @@ func applySpinloopEnv(sel spinloop.Selection, spinloopPath string) error { // envFlagUsage is the --env flag's help text on every remote subcommand that // acts on one environment. -const envFlagUsage = "the registered environment to act on (defaults to the default environment)" +const envFlagUsage = "the registered environment to act on (required)" // resolveRemoteConfig loads the remote config a subcommand acts on. The --env // flag names an environment in the per-user registry, read from @@ -125,23 +126,34 @@ func resolveRemoteConfig(envName, spinloopArg string) (remote.Config, error) { return remote.Config{}, err } } - if envName != "" { - if !remote.IsEnvName(envName) { - return remote.Config{}, fmt.Errorf("%q is not an environment name: an environment name is a plain identifier, with no path", envName) - } - path, err := remote.EnvConfigPath(envName) - if err != nil { - return remote.Config{}, err - } - if _, err := os.Stat(path); err != nil { - if os.IsNotExist(err) { - return remote.Config{}, fmt.Errorf("environment %q is not registered: run `spinloop remote deploy --env %q` to create it", envName, envName) - } - return remote.Config{}, err - } - return remote.LoadConfigFile(path, viperGetenv()) + if envName == "" { + return remote.Config{}, errNoEnvironment() + } + if !remote.IsEnvName(envName) { + return remote.Config{}, fmt.Errorf("%q is not an environment name: an environment name is a plain identifier, with no path", envName) + } + return remote.LoadEnvironment(envName, viperGetenv()) +} + +// errNoEnvironment is what a remote subcommand fails with when it names no +// environment. There is no environment to fall back to: several of these +// subcommands change the state of a cloud instance, and one nobody named is +// not one to start, stop or terminate. The registered names are listed, so the +// next thing to type is in the error rather than a directory listing away. +func errNoEnvironment() error { + var b strings.Builder + b.WriteString("no environment named: pass --env ") + envs, err := remote.ListEnvironments() + if err != nil || len(envs) == 0 { + b.WriteString(" (none registered yet — `spinloop remote deploy --env ` creates one)") + return errors.New(b.String()) + } + names := make([]string, len(envs)) + for i, e := range envs { + names[i] = e.Name } - return remote.LoadDefault(viperGetenv()) + fmt.Fprintf(&b, " (registered: %s)", strings.Join(names, ", ")) + return errors.New(b.String()) } // defaultSpinloopNamed reports whether there is a default Spinloop for readSpinloop diff --git a/cmd/spinloop/remote_deploy_test.go b/cmd/spinloop/remote_deploy_test.go index 042fd29a..c2fd55cb 100644 --- a/cmd/spinloop/remote_deploy_test.go +++ b/cmd/spinloop/remote_deploy_test.go @@ -853,7 +853,7 @@ func TestRemoteStart_ReportsProgressWhileWaiting(t *testing.T) { writeRemoteConfig(t, server.URL) stderr := captureStderr(t, func() { - if err := cmdRemoteStart(nil); err != nil { + if err := cmdRemoteStart([]string{"--env", "default"}); err != nil { t.Errorf("cmdRemoteStart: %v", err) } }) @@ -1022,7 +1022,7 @@ func TestRemoteStart_HeartbeatTracksTheCapacityWaitEnding(t *testing.T) { writeRemoteConfig(t, server.URL) stderr := captureStderr(t, func() { - if err := cmdRemoteStart(nil); err != nil { + if err := cmdRemoteStart([]string{"--env", "default"}); err != nil { t.Fatalf("cmdRemoteStart: %v", err) } }) @@ -1069,7 +1069,7 @@ func TestRemoteStart_TimeoutShorthand(t *testing.T) { defer func() { os.Stderr = oldStderr; w.Close() }() done := make(chan error, 1) - go func() { done <- cmdRemoteStart([]string{"-t", "80ms"}) }() + go func() { done <- cmdRemoteStart([]string{"--env", "default", "-t", "80ms"}) }() select { case err := <-done: if err == nil { @@ -1093,7 +1093,7 @@ func TestRemoteStart_StdoutCarriesOnlyTheResult(t *testing.T) { writeRemoteConfig(t, server.URL) out := captureStdout(t, func() { - if err := cmdRemoteStart([]string{"--print-env"}); err != nil { + if err := cmdRemoteStart([]string{"--env", "default", "--print-env"}); err != nil { t.Errorf("cmdRemoteStart: %v", err) } }) diff --git a/cmd/spinloop/remote_environments_test.go b/cmd/spinloop/remote_environments_test.go index 063a6934..8c7634eb 100644 --- a/cmd/spinloop/remote_environments_test.go +++ b/cmd/spinloop/remote_environments_test.go @@ -9,6 +9,7 @@ import ( "strings" "testing" + "github.com/spinloop-ai/spinloop/internal/config" "github.com/spinloop-ai/spinloop/internal/remote" ) @@ -67,7 +68,7 @@ func TestRemote_DefaultEnvironment(t *testing.T) { t.Chdir(t.TempDir()) // no ./Spinloop here out := captureStdout(t, func() { - if err := cmdRemoteStatus(nil); err != nil { + if err := cmdRemoteStatus([]string{"--env", "default"}); err != nil { t.Errorf("status via default env: %v", err) } }) @@ -76,28 +77,29 @@ func TestRemote_DefaultEnvironment(t *testing.T) { } } -// A pre-existing ~/.config/spinloop/remote.json is read as the default env. -func TestRemote_LegacyFileReadThrough(t *testing.T) { +// A file at the superseded path configures nothing: no path outside the +// registry is read for any name. +func TestRemote_SupersededFileIsNotRead(t *testing.T) { isolateConfig(t) stubAWSEnv(t) server := stateServer(t) defer server.Close() data, _ := json.Marshal(remote.Config{StartURL: server.URL, StopURL: server.URL, Region: "eu-west-1"}) - if err := os.MkdirAll(filepath.Dir(must1(remote.ConfigPath())), 0o700); err != nil { + home := must1(config.Dir()) + if err := os.MkdirAll(home, 0o700); err != nil { t.Fatal(err) } - if err := os.WriteFile(must1(remote.ConfigPath()), data, 0o600); err != nil { + if err := os.WriteFile(filepath.Join(home, "remote.json"), data, 0o600); err != nil { t.Fatal(err) } t.Chdir(t.TempDir()) - out := captureStdout(t, func() { - if err := cmdRemoteStatus(nil); err != nil { - t.Errorf("status via legacy file: %v", err) - } - }) - if !strings.Contains(out, "state: running") { - t.Errorf("legacy remote.json should be read as default, got:\n%s", out) + err := cmdRemoteMetrics([]string{"--env", "default"}) + if err == nil { + t.Fatal("the superseded file must not configure an environment") + } + if !strings.Contains(err.Error(), "remotes/default/remote.json") { + t.Errorf("the failure should name the registry path, got %v", err) } } diff --git a/cmd/spinloop/remote_test.go b/cmd/spinloop/remote_test.go index f4d07283..58201baf 100644 --- a/cmd/spinloop/remote_test.go +++ b/cmd/spinloop/remote_test.go @@ -29,10 +29,11 @@ func stubAWSEnv(t *testing.T) { t.Setenv("AWS_EC2_METADATA_DISABLED", "true") } -// writeRemoteConfig stores a remote config pointing at the test server. +// writeRemoteConfig registers the `default` environment pointing at the test +// server — the registry is the only place a configuration is read from. func writeRemoteConfig(t *testing.T, serverURL string) { t.Helper() - path := must1(remote.ConfigPath()) + path := must1(remote.EnvConfigPath("default")) if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { t.Fatal(err) } @@ -68,8 +69,20 @@ func TestRemoteDispatch(t *testing.T) { func TestRemote_Unconfigured(t *testing.T) { isolateConfig(t) - for _, sub := range []string{"start", "restart", "stop", "status"} { // deploy needs a Spinloop, covered separately - if err := run([]string{"remote", sub}); err == nil || !strings.Contains(err.Error(), "not configured") { + // deploy needs a Spinloop, covered separately. + subs := []string{"start", "restart", "stop", "metrics"} + // Naming no environment is its own failure: these commands act on one + // instance, and an instance nobody named is not one to act on. + for _, sub := range subs { + err := run([]string{"remote", sub}) + if err == nil || !strings.Contains(err.Error(), "pass --env") { + t.Errorf("remote %s with no environment should name the flag, got %v", sub, err) + } + } + // Naming one that has no configuration explains the setup. + for _, sub := range subs { + err := run([]string{"remote", sub, "--env", "default"}) + if err == nil || !strings.Contains(err.Error(), "not configured") { t.Errorf("remote %s without config should explain setup, got %v", sub, err) } } @@ -86,7 +99,7 @@ func TestRemoteStart_PrintsExports(t *testing.T) { writeRemoteConfig(t, server.URL) out := captureStdout(t, func() { - if err := cmdRemoteStart([]string{"--print-env"}); err != nil { + if err := cmdRemoteStart([]string{"--env", "default", "--print-env"}); err != nil { t.Errorf("cmdRemoteStart: %v", err) } }) @@ -108,7 +121,7 @@ func TestRemoteStart_NoExportsWithoutFlag(t *testing.T) { writeRemoteConfig(t, server.URL) out := captureStdout(t, func() { - if err := cmdRemoteStart(nil); err != nil { + if err := cmdRemoteStart([]string{"--env", "default"}); err != nil { t.Errorf("cmdRemoteStart: %v", err) } }) @@ -163,7 +176,7 @@ func TestRemoteEnv_PrintsExports(t *testing.T) { writeRemoteConfig(t, server.URL) out := captureStdout(t, func() { - if err := cmdRemoteEnv(nil); err != nil { + if err := cmdRemoteEnv([]string{"--env", "default"}); err != nil { t.Errorf("cmdRemoteEnv: %v", err) } }) @@ -184,7 +197,7 @@ func TestRemoteStatus_PrintsState(t *testing.T) { writeRemoteConfig(t, server.URL) out := captureStdout(t, func() { - if err := cmdRemoteStatus(nil); err != nil { + if err := cmdRemoteStatus([]string{"--env", "default"}); err != nil { t.Errorf("cmdRemoteStatus: %v", err) } }) @@ -210,7 +223,7 @@ func TestRemoteStatus_PrintsVersion(t *testing.T) { } })) defer server.Close() - path := must1(remote.ConfigPath()) + path := must1(remote.EnvConfigPath("default")) if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { t.Fatal(err) } @@ -229,7 +242,7 @@ func TestRemoteStatus_PrintsVersion(t *testing.T) { } out := captureStdout(t, func() { - if err := cmdRemoteStatus(nil); err != nil { + if err := cmdRemoteStatus([]string{"--env", "default"}); err != nil { t.Errorf("cmdRemoteStatus: %v", err) } }) @@ -252,7 +265,7 @@ func TestRemoteStop_PrintsState(t *testing.T) { writeRemoteConfig(t, server.URL) out := captureStdout(t, func() { - if err := cmdRemoteStop(nil); err != nil { + if err := cmdRemoteStop([]string{"--env", "default"}); err != nil { t.Errorf("cmdRemoteStop: %v", err) } }) @@ -277,7 +290,7 @@ func TestRemotePause_PrintsState(t *testing.T) { writeRemoteConfig(t, server.URL) out := captureStdout(t, func() { - if err := cmdRemotePause(nil); err != nil { + if err := cmdRemotePause([]string{"--env", "default"}); err != nil { t.Errorf("cmdRemotePause: %v", err) } }) @@ -323,7 +336,7 @@ func TestRemoteRestart_Flow(t *testing.T) { writeRemoteConfig(t, server.URL) out := captureStdout(t, func() { - if err := run([]string{"remote", "restart"}); err != nil { + if err := run([]string{"remote", "restart", "--env", "default"}); err != nil { t.Errorf("remote restart: %v", err) } }) @@ -351,7 +364,7 @@ func TestRemoteRestart_ForceFlag(t *testing.T) { writeRemoteConfig(t, server.URL) out := captureStdout(t, func() { - if err := cmdRemoteRestart([]string{flag}); err != nil { + if err := cmdRemoteRestart([]string{"--env", "default", flag}); err != nil { t.Errorf("remote restart %s: %v", flag, err) } }) @@ -379,7 +392,7 @@ func TestRemoteRestart_TimeoutFlag(t *testing.T) { writeRemoteConfig(t, server.URL) out := captureStdout(t, func() { - if err := cmdRemoteRestart([]string{"--timeout", "5m"}); err != nil { + if err := cmdRemoteRestart([]string{"--env", "default", "--timeout", "5m"}); err != nil { t.Errorf("remote restart --timeout: %v", err) } }) @@ -400,7 +413,7 @@ func TestRemoteRestart_AlreadyStoppedBehavesAsStart(t *testing.T) { writeRemoteConfig(t, server.URL) out := captureStdout(t, func() { - if err := cmdRemoteRestart(nil); err != nil { + if err := cmdRemoteRestart([]string{"--env", "default"}); err != nil { t.Errorf("remote restart on a stopped environment: %v", err) } }) @@ -436,7 +449,7 @@ func TestRemoteRestart_StatusFailureDoesNotGate(t *testing.T) { out := captureStdout(t, func() { errOut := captureStderr(t, func() { - if err := cmdRemoteRestart(nil); err != nil { + if err := cmdRemoteRestart([]string{"--env", "default"}); err != nil { t.Errorf("remote restart after a failed status check: %v", err) } }) @@ -467,7 +480,7 @@ func TestRemoteRestart_WakeFailureReportsRecovery(t *testing.T) { defer server.Close() writeRemoteConfig(t, server.URL) - err := cmdRemoteRestart(nil) + err := cmdRemoteRestart([]string{"--env", "default"}) if err == nil { t.Fatal("expected a wake failure error") } @@ -513,7 +526,7 @@ func TestRemote_SpinloopDiscovery(t *testing.T) { } out := captureStdout(t, func() { - if err := cmdRemoteStatus(nil); err != nil { + if err := cmdRemoteStatus([]string{"--env", "default"}); err != nil { t.Errorf("cmdRemoteStatus: %v", err) } }) @@ -532,7 +545,7 @@ func TestRemote_ExplicitSpinloopDoesNotNameAnEnvironment(t *testing.T) { if err := os.WriteFile("Spinloop", []byte("PROVIDER ollama\n"), 0o600); err != nil { t.Fatal(err) } - err := cmdRemoteStatus([]string{"Spinloop"}) + err := cmdRemoteStatus([]string{"--env", "default", "Spinloop"}) if err == nil || !strings.Contains(err.Error(), "remote is not configured") { t.Errorf("want the not-configured error, got %v", err) } @@ -553,7 +566,7 @@ func TestRemote_SpinloopFallsBackToTheUserConfig(t *testing.T) { t.Fatal(err) } out := captureStdout(t, func() { - if err := cmdRemoteStatus(nil); err != nil { + if err := cmdRemoteStatus([]string{"--env", "default"}); err != nil { t.Errorf("cmdRemoteStatus: %v", err) } }) @@ -580,7 +593,7 @@ func TestRemote_IgnoresLowercaseSpinloopFile(t *testing.T) { t.Fatal(err) } out := captureStdout(t, func() { - if err := cmdRemoteStatus(nil); err != nil { + if err := cmdRemoteStatus([]string{"--env", "default"}); err != nil { t.Errorf("cmdRemoteStatus: %v", err) } }) @@ -625,7 +638,7 @@ func TestRemoteMetrics_Running(t *testing.T) { t.Setenv("SPINLOOP_REMOTE_STATS_URL", server.URL) out := captureStdout(t, func() { - if err := cmdRemoteMetrics([]string{"--format=table"}); err != nil { + if err := cmdRemoteMetrics([]string{"--env", "default", "--format=table"}); err != nil { t.Errorf("cmdRemoteMetrics: %v", err) } }) @@ -669,7 +682,7 @@ func TestRemoteMetrics_Stopped(t *testing.T) { t.Setenv("SPINLOOP_REMOTE_STATS_URL", server.URL) out := captureStdout(t, func() { - if err := cmdRemoteMetrics([]string{"--format=table"}); err != nil { + if err := cmdRemoteMetrics([]string{"--env", "default", "--format=table"}); err != nil { t.Errorf("cmdRemoteMetrics: %v", err) } }) @@ -698,7 +711,7 @@ func TestRemoteMetrics_WithErrors(t *testing.T) { t.Setenv("SPINLOOP_REMOTE_STATS_URL", server.URL) errOut := captureStderr(t, func() { - if err := cmdRemoteMetrics(nil); err != nil { + if err := cmdRemoteMetrics([]string{"--env", "default"}); err != nil { t.Errorf("cmdRemoteMetrics: %v", err) } }) @@ -733,7 +746,7 @@ func TestRemoteMetrics_DefaultFormat(t *testing.T) { t.Setenv("SPINLOOP_REMOTE_STATS_URL", server.URL) out := captureStdout(t, func() { - if err := cmdRemoteMetrics(nil); err != nil { + if err := cmdRemoteMetrics([]string{"--env", "default"}); err != nil { t.Errorf("cmdRemoteMetrics: %v", err) } }) @@ -770,7 +783,7 @@ func TestRemoteMetrics_JsonFormat(t *testing.T) { t.Setenv("SPINLOOP_REMOTE_STATS_URL", server.URL) out := captureStdout(t, func() { - if err := cmdRemoteMetrics([]string{"--format=json"}); err != nil { + if err := cmdRemoteMetrics([]string{"--env", "default", "--format=json"}); err != nil { t.Errorf("cmdRemoteMetrics: %v", err) } }) @@ -807,7 +820,7 @@ func TestRemoteMetrics_JsonFormatWithCost(t *testing.T) { t.Setenv("SPINLOOP_REMOTE_STATS_URL", server.URL) out := captureStdout(t, func() { - if err := cmdRemoteMetrics([]string{"--format=json", "--cost"}); err != nil { + if err := cmdRemoteMetrics([]string{"--env", "default", "--format=json", "--cost"}); err != nil { t.Errorf("cmdRemoteMetrics: %v", err) } }) @@ -825,7 +838,7 @@ func TestRemoteMetrics_InvalidFormat(t *testing.T) { stubAWSEnv(t) writeRemoteConfig(t, "http://localhost:0") - err := cmdRemoteMetrics([]string{"--format=csv"}) + err := cmdRemoteMetrics([]string{"--env", "default", "--format=csv"}) if err == nil || !strings.Contains(err.Error(), "format") { t.Errorf("expected format error, got %v", err) } @@ -855,7 +868,7 @@ func TestRemoteMetrics_BarFormat(t *testing.T) { t.Setenv("SPINLOOP_REMOTE_STATS_URL", server.URL) out := captureStdout(t, func() { - err := cmdRemoteMetrics([]string{"--format=bar"}) + err := cmdRemoteMetrics([]string{"--env", "default", "--format=bar"}) if err != nil { t.Errorf("bar format failed: %v", err) } @@ -900,7 +913,7 @@ func TestRemoteMetrics_BarFormatStopped(t *testing.T) { t.Setenv("SPINLOOP_REMOTE_STATS_URL", server.URL) out := captureStdout(t, func() { - err := cmdRemoteMetrics([]string{"--format=bar"}) + err := cmdRemoteMetrics([]string{"--env", "default", "--format=bar"}) if err != nil { t.Errorf("bar format failed: %v", err) } @@ -976,7 +989,7 @@ func TestRemoteMetrics_WatchMode(t *testing.T) { defer func() { metricsWatchInterval = oldInterval }() out := captureStdout(t, func() { - err := cmdRemoteMetrics([]string{"--watch", "--format=table"}) + err := cmdRemoteMetrics([]string{"--env", "default", "--watch", "--format=table"}) // Expects error from the 3rd call. if err == nil { t.Error("watch should exit with error when server fails") @@ -1016,7 +1029,7 @@ func TestRemoteMetrics_WatchShortFlag(t *testing.T) { defer func() { metricsWatchInterval = oldInterval }() out := captureStdout(t, func() { - err := cmdRemoteMetrics([]string{"-w", "--format=table"}) + err := cmdRemoteMetrics([]string{"--env", "default", "-w", "--format=table"}) if err == nil { t.Error("-w should exit with error when server fails") } @@ -1048,7 +1061,7 @@ func TestRemoteMetrics_MultiGPU(t *testing.T) { t.Setenv("SPINLOOP_REMOTE_STATS_URL", server.URL) out := captureStdout(t, func() { - if err := cmdRemoteMetrics([]string{"--format=table"}); err != nil { + if err := cmdRemoteMetrics([]string{"--env", "default", "--format=table"}); err != nil { t.Errorf("cmdRemoteMetrics: %v", err) } }) @@ -1071,7 +1084,7 @@ func TestRemoteMetrics_JsonStopped(t *testing.T) { t.Setenv("SPINLOOP_REMOTE_STATS_URL", server.URL) out := captureStdout(t, func() { - if err := cmdRemoteMetrics([]string{"--format=json"}); err != nil { + if err := cmdRemoteMetrics([]string{"--env", "default", "--format=json"}); err != nil { t.Errorf("cmdRemoteMetrics: %v", err) } }) @@ -1101,7 +1114,7 @@ func TestRemoteMetrics_JsonWithErrors(t *testing.T) { t.Setenv("SPINLOOP_REMOTE_STATS_URL", server.URL) out := captureStdout(t, func() { - if err := cmdRemoteMetrics([]string{"--format=json"}); err != nil { + if err := cmdRemoteMetrics([]string{"--env", "default", "--format=json"}); err != nil { t.Errorf("cmdRemoteMetrics: %v", err) } }) @@ -1147,7 +1160,7 @@ func TestRemoteStart_ProbeSucceedsNoWarning(t *testing.T) { writeRemoteConfig(t, server.URL) stderr := captureStderr(t, func() { - if err := cmdRemoteStart(nil); err != nil { + if err := cmdRemoteStart([]string{"--env", "default"}); err != nil { t.Errorf("cmdRemoteStart: %v", err) } }) @@ -1178,7 +1191,7 @@ func TestRemoteStart_ProbeFailsWarns(t *testing.T) { writeRemoteConfig(t, server.URL) errOut := captureStderr(t, func() { - err := cmdRemoteStart(nil) + err := cmdRemoteStart([]string{"--env", "default"}) if err != nil { t.Fatalf("start should exit 0 after a probe warning, got %v", err) } @@ -1214,7 +1227,7 @@ func TestRemoteStart_ProbeFailsIPDetectFails(t *testing.T) { writeRemoteConfig(t, server.URL) errOut := captureStderr(t, func() { - err := cmdRemoteStart(nil) + err := cmdRemoteStart([]string{"--env", "default"}) if err != nil { t.Fatalf("start should exit 0 even when probe and IP detection both fail, got %v", err) } @@ -1256,7 +1269,7 @@ func TestRemoteMetrics_WatchBuffersBeforeClear(t *testing.T) { defer func() { metricsWatchInterval = oldInterval }() out := captureStdout(t, func() { - cmdRemoteMetrics([]string{"--watch", "--format=table"}) + cmdRemoteMetrics([]string{"--env", "default", "--watch", "--format=table"}) }) // The clear-screen escape sequence. @@ -1309,7 +1322,7 @@ func TestRemoteKeep_PrintsDeadline(t *testing.T) { writeRemoteConfig(t, server.URL) // Also need to write the update URL. - path := must1(remote.ConfigPath()) + path := must1(remote.EnvConfigPath("default")) data := must1(os.ReadFile(path)) var cfg remote.Config json.Unmarshal(data, &cfg) @@ -1317,7 +1330,7 @@ func TestRemoteKeep_PrintsDeadline(t *testing.T) { os.WriteFile(path, must1(json.Marshal(cfg)), 0o600) out := captureStdout(t, func() { - if err := cmdRemoteKeep([]string{"4h"}); err != nil { + if err := cmdRemoteKeep([]string{"--env", "default", "4h"}); err != nil { t.Errorf("cmdRemoteKeep: %v", err) } }) @@ -1328,7 +1341,7 @@ func TestRemoteKeep_PrintsDeadline(t *testing.T) { // TestRemoteKeep_MissingDuration fails. func TestRemoteKeep_MissingDuration(t *testing.T) { - err := cmdRemoteKeep(nil) + err := cmdRemoteKeep([]string{"--env", "default"}) if err == nil || !strings.Contains(err.Error(), "usage") { t.Errorf("expected usage error, got %v", err) } @@ -1336,7 +1349,7 @@ func TestRemoteKeep_MissingDuration(t *testing.T) { // TestRemoteKeep_InvalidDuration fails. func TestRemoteKeep_InvalidDuration(t *testing.T) { - err := cmdRemoteKeep([]string{"4hours"}) + err := cmdRemoteKeep([]string{"--env", "default", "4hours"}) if err == nil || !strings.Contains(err.Error(), "invalid duration") { t.Errorf("expected duration parse error, got %v", err) } @@ -1357,7 +1370,7 @@ func TestRemoteStart_KeepFlag(t *testing.T) { // Probe reachability will fail, but that's stderr and doesn't affect the test. out := captureStdout(t, func() { - cmdRemoteStart([]string{"--keep", "2h"}) + cmdRemoteStart([]string{"--env", "default", "--keep", "2h"}) }) // The keep deadline should be reported on stderr (via progress). // We can check that the request included the retainUntil parameter. diff --git a/cmd/spinloop/retain_render_test.go b/cmd/spinloop/retain_render_test.go index e1d10710..2e482a87 100644 --- a/cmd/spinloop/retain_render_test.go +++ b/cmd/spinloop/retain_render_test.go @@ -58,7 +58,7 @@ func TestRemoteMetricsBarKeepsOnTheActiveLine(t *testing.T) { }`) out := captureStdout(t, func() { - if err := cmdRemoteMetrics([]string{"--format=bar"}); err != nil { + if err := cmdRemoteMetrics([]string{"--env", "default", "--format=bar"}); err != nil { t.Fatalf("cmdRemoteMetrics: %v", err) } }) @@ -89,7 +89,7 @@ func TestRemoteMetricsTableKeepsOnTheActiveRow(t *testing.T) { }`) out := captureStdout(t, func() { - if err := cmdRemoteMetrics([]string{"--format=table"}); err != nil { + if err := cmdRemoteMetrics([]string{"--env", "default", "--format=table"}); err != nil { t.Fatalf("cmdRemoteMetrics: %v", err) } }) @@ -123,7 +123,7 @@ func TestKeepDurationRendersRelatively(t *testing.T) { "retainUntil": "`+deadline+`" }`) out := captureStdout(t, func() { - if err := cmdRemoteMetrics([]string{"--format=bar"}); err != nil { + if err := cmdRemoteMetrics([]string{"--env", "default", "--format=bar"}); err != nil { t.Fatalf("cmdRemoteMetrics: %v", err) } }) @@ -149,7 +149,7 @@ func TestRemoteMetricsStoppedKeptStillShowsKeep(t *testing.T) { "retainUntil": "`+deadline+`" }`) out := captureStdout(t, func() { - if err := cmdRemoteMetrics([]string{"--format=" + format}); err != nil { + if err := cmdRemoteMetrics([]string{"--env", "default", "--format=" + format}); err != nil { t.Fatalf("cmdRemoteMetrics: %v", err) } }) @@ -173,7 +173,7 @@ func TestRemoteMetricsOmitsKeepWhenAbsent(t *testing.T) { "idleSeconds": 125 }`) out := captureStdout(t, func() { - if err := cmdRemoteMetrics([]string{"--format=" + format}); err != nil { + if err := cmdRemoteMetrics([]string{"--env", "default", "--format=" + format}); err != nil { t.Fatalf("cmdRemoteMetrics: %v", err) } }) diff --git a/cmd/spinloop/viper_test.go b/cmd/spinloop/viper_test.go index 0e78a582..2d67fbc5 100644 --- a/cmd/spinloop/viper_test.go +++ b/cmd/spinloop/viper_test.go @@ -104,7 +104,7 @@ func TestViperRemoteEnvPrecedence(t *testing.T) { } // Unset variables fall through to the file. - cfg, err := resolveRemoteConfig("", "") + cfg, err := resolveRemoteConfig("default", "") if err != nil { t.Fatalf("resolveRemoteConfig: %v", err) } @@ -117,7 +117,7 @@ func TestViperRemoteEnvPrecedence(t *testing.T) { // Each exported variable wins over the file, one at a time. for name, get := range legs { t.Setenv(name, envValue) - cfg, err := resolveRemoteConfig("", "") + cfg, err := resolveRemoteConfig("default", "") if err != nil { t.Fatalf("%s set: %v", name, err) } diff --git a/docs/commands/remote.md b/docs/commands/remote.md index d2d6f253..0be29f67 100644 --- a/docs/commands/remote.md +++ b/docs/commands/remote.md @@ -130,11 +130,14 @@ deployment state per-user and per-machine: two projects name two environments without clobbering, and the Spinloop carries none of the URLs at all. `spinloop remote deploy` registers an environment for you; you can also create one by hand. A name is a plain identifier — `--env ./remote.json` fails, -saying an environment name has no path. With no flag, a command uses the -`default` environment (`~/.config/spinloop/remotes/default/remote.json`), so it -works from anywhere. An existing `~/.config/spinloop/remote.json` from before -the registry is still read as the default; move it to -`remotes/default/remote.json` when convenient. +saying an environment name has no path. + +**`--env` is required.** There is no environment a command falls back to: half +of these subcommands start, stop or terminate a cloud instance, and an instance +nobody named is not one to act on. A command given no `--env` fails naming the +flag and listing the environments you have registered, so the next thing to +type is in the error. `default` is an ordinary name — call an environment that +if you like, and pass `--env default` to use it. A command may also be given a Spinloop path (or a [registered alias](alias.md), or a URL): its `ENV` lines and the `.env` beside it are read diff --git a/docs/env-vars.md b/docs/env-vars.md index 6dc381af..419fab8d 100644 --- a/docs/env-vars.md +++ b/docs/env-vars.md @@ -33,11 +33,15 @@ from the environment or a `.env` beside the Spinloop — never written into an | `SPINLOOP_REMOTE_REGION` | Override the AWS region (else `AWS_REGION`, else the region in the Function URL host). | | `SPINLOOP_REMOTE_PACKAGE_MANAGER` | Pin the package manager (`pnpm`/`npm`) `spinloop remote bootstrap` and `bake` use. | -These let the remote commands run without a `remote.json` on disk — the -config can come entirely from the environment. `spinloop remote logs` is the -exception: it needs the environment's name to find its log streams, and that -comes only from the config, so it wants a registered environment (or a Spinloop -naming one) rather than environment variables alone. +These let the remote commands run without a `remote.json` on disk — the config +can come entirely from the environment. `--env ` is still required, and +on this path the name you give *is* the environment identifier the control +plane acts on, since there is no file to take one from: + +```sh +SPINLOOP_REMOTE_START_URL=... SPINLOOP_REMOTE_STOP_URL=... SPINLOOP_REMOTE_REGION=... \ + spinloop remote start --env ci +``` ## Standard variables spinloop honours diff --git a/internal/fleet/node.go b/internal/fleet/node.go index e7cff665..3d765772 100644 --- a/internal/fleet/node.go +++ b/internal/fleet/node.go @@ -128,12 +128,9 @@ func (n *daemonNode) Logs(ctx context.Context, offset int64, limit int) (daemon. // row rather than a blanked view. func (c *Config) NewNode(entry NodeConfig) (Node, error) { if entry.Kind == KindRemote { - // The node's name is the registered environment's key. - path, err := remote.EnvConfigPath(entry.Name) - if err != nil { - return nil, err - } - cfg, err := remote.LoadConfigFile(path, os.Getenv) + // The node's name is the registered environment's key, and every + // environment resolves the one way: by name, from the registry. + cfg, err := remote.LoadEnvironment(entry.Name, os.Getenv) if err != nil { return nil, err } diff --git a/internal/remote/environments.go b/internal/remote/environments.go index 0baa8575..a7df9435 100644 --- a/internal/remote/environments.go +++ b/internal/remote/environments.go @@ -2,7 +2,6 @@ package remote import ( "encoding/json" - "fmt" "os" "path/filepath" "strings" @@ -130,34 +129,3 @@ func SaveEnvironment(name string, cfg Config) error { } return os.WriteFile(filepath.Join(dir, "remote.json"), append(data, '\n'), 0o600) } - -// LoadDefault loads the remote config used when no Spinloop names an environment: -// the `default` environment, falling back to the legacy single per-user file -// (~/.config/spinloop/remote.json) for setups that predate the registry. As with -// LoadConfig a missing file is not fatal — environment variables alone may carry -// the config — and finishConfig reports where to put it otherwise. -func LoadDefault(getenv func(string) string) (Config, error) { - defaultPath, err := EnvConfigPath("default") - if err != nil { - return Config{}, err - } - legacyPath, err := ConfigPath() - if err != nil { - return Config{}, err - } - for _, path := range []string{defaultPath, legacyPath} { - data, err := os.ReadFile(path) - if err != nil { - if os.IsNotExist(err) { - continue - } - return Config{}, err - } - var cfg Config - if err := json.Unmarshal(data, &cfg); err != nil { - return Config{}, fmt.Errorf("parsing %s: %w", path, err) - } - return finishConfig(cfg, getenv, path) - } - return finishConfig(Config{}, getenv, defaultPath) -} diff --git a/internal/remote/environments_test.go b/internal/remote/environments_test.go index 0f5f98e3..9e177687 100644 --- a/internal/remote/environments_test.go +++ b/internal/remote/environments_test.go @@ -3,6 +3,7 @@ package remote import ( "os" "path/filepath" + "strings" "testing" ) @@ -96,7 +97,7 @@ func TestSaveEnvironment(t *testing.T) { t.Errorf("remote.json mode = %v, want 0600", fi.Mode().Perm()) } // Round-trips through the loader, environment identifier included. - got, err := LoadConfigFile(must1(EnvConfigPath("prod")), func(string) string { return "" }) + got, err := LoadEnvironment("prod", func(string) string { return "" }) if err != nil { t.Fatal(err) } @@ -113,38 +114,61 @@ func TestSaveEnvironment(t *testing.T) { } } -func TestLoadDefault(t *testing.T) { +func TestLoadEnvironment_ByName(t *testing.T) { getenv := func(string) string { return "" } cfg := `{"start_url":"https://s","stop_url":"https://x","region":"eu-west-1"}` - t.Run("default environment", func(t *testing.T) { + t.Run("reads the named environment", func(t *testing.T) { t.Setenv("XDG_CONFIG_HOME", t.TempDir()) writeEnv(t, "default", cfg) - got, err := LoadDefault(getenv) + got, err := LoadEnvironment("default", getenv) if err != nil || got.StartURL != "https://s" { - t.Fatalf("default env: %+v, %v", got, err) + t.Fatalf("named env: %+v, %v", got, err) } }) - t.Run("legacy file fallback", func(t *testing.T) { + // The superseded path is not a second place an environment can live: a + // file there is simply not read, whatever it holds. + t.Run("a file at the superseded path is not read", func(t *testing.T) { home := t.TempDir() t.Setenv("XDG_CONFIG_HOME", home) - if err := os.MkdirAll(filepath.Dir(must1(ConfigPath())), 0o700); err != nil { + legacy := filepath.Join(home, "spinloop", "remote.json") + if err := os.MkdirAll(filepath.Dir(legacy), 0o700); err != nil { t.Fatal(err) } - if err := os.WriteFile(must1(ConfigPath()), []byte(cfg), 0o600); err != nil { + if err := os.WriteFile(legacy, []byte(cfg), 0o600); err != nil { t.Fatal(err) } - got, err := LoadDefault(getenv) - if err != nil || got.Region != "eu-west-1" { - t.Fatalf("legacy fallback: %+v, %v", got, err) + if _, err := LoadEnvironment("default", getenv); err == nil { + t.Fatal("the superseded path must not configure an environment") } }) - t.Run("neither present reports where to put it", func(t *testing.T) { + t.Run("nothing present reports where to put it", func(t *testing.T) { t.Setenv("XDG_CONFIG_HOME", t.TempDir()) - if _, err := LoadDefault(getenv); err == nil { - t.Fatal("expected an error naming the default environment") + _, err := LoadEnvironment("default", getenv) + if err == nil { + t.Fatal("expected an error naming the environment's path") + } + if !strings.Contains(err.Error(), "remotes/default/remote.json") { + t.Errorf("the failure should name the registry path, got %v", err) + } + }) + + // The documented no-file workflow: the overrides carry the configuration, + // and the name carries the identifier the control calls need. + t.Run("overrides configure a named environment with no file", func(t *testing.T) { + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + got, err := LoadEnvironment("ci", envMap(map[string]string{ + "SPINLOOP_REMOTE_START_URL": "https://s", + "SPINLOOP_REMOTE_STOP_URL": "https://x", + "SPINLOOP_REMOTE_REGION": "eu-west-1", + })) + if err != nil { + t.Fatalf("overrides should configure it: %v", err) + } + if got.Environment != "ci" { + t.Errorf("Environment = %q, want the name given", got.Environment) } }) } diff --git a/internal/remote/remote.go b/internal/remote/remote.go index bdcafd62..8cdf5444 100644 --- a/internal/remote/remote.go +++ b/internal/remote/remote.go @@ -17,7 +17,6 @@ import ( "net/http" "net/url" "os" - "path/filepath" "regexp" "strings" "time" @@ -69,58 +68,36 @@ type Config struct { Environment string `json:"environment"` } -// ConfigPath returns the path of the legacy per-user remote config file, -// alongside spinloop's own config in the same directory. The environments -// registry (see environments.go) supersedes it; it is still read as the -// fallback for the default environment. -func ConfigPath() (string, error) { - home, err := ConfigHome() - if err != nil { - return "", err - } - return filepath.Join(home, "remote.json"), nil -} - -// LoadConfig reads the per-user config file and applies environment overrides -// (SPINLOOP_REMOTE_START_URL, SPINLOOP_REMOTE_STOP_URL, SPINLOOP_REMOTE_REGION; the -// region also falls back to AWS_REGION and then to the region embedded in the -// Function URL host). A missing file is fine — env vars alone can carry the -// config. getenv is injectable for tests. -func LoadConfig(getenv func(string) string) (Config, error) { - path, err := ConfigPath() +// LoadEnvironment reads a named environment's configuration: the remote.json +// in its registry directory, with the SPINLOOP_REMOTE_* overrides applied on +// top. It is the only way an environment is resolved — there is no path +// outside the registry, and no name that resolves by a different rule. +// +// A name with no file is not a failure on its own: the overrides may carry a +// complete configuration, which is how the remote commands run on a machine +// with nothing on disk. In that case the name is the environment identifier +// the control calls carry, since a configuration assembled from variables has +// no file to take one from. Where the overrides are incomplete too, +// finishConfig fails naming the registry path to create. +func LoadEnvironment(name string, getenv func(string) string) (Config, error) { + path, err := EnvConfigPath(name) if err != nil { return Config{}, err } var cfg Config data, err := os.ReadFile(path) - if err == nil { + switch { + case err == nil: if err := json.Unmarshal(data, &cfg); err != nil { return Config{}, fmt.Errorf("parsing %s: %w", path, err) } - } else if !os.IsNotExist(err) { - return Config{}, err - } - return finishConfig(cfg, getenv, path) -} - -// LoadConfigFile reads a registered environment's remote.json — the file -// named by an environment in the per-user registry — then applies the same -// environment overrides as LoadConfig. Unlike LoadConfig, the file must -// exist: it was asked for by name. -func LoadConfigFile(path string, getenv func(string) string) (Config, error) { - data, err := os.ReadFile(path) - if err != nil { - if os.IsNotExist(err) { - return Config{}, fmt.Errorf( - "remote config %s does not exist: run `spinloop remote deploy` to create and register the environment", - path) - } + case os.IsNotExist(err): + // No file: the overrides may still configure it, and the name is what + // tells the shared Lambdas which instance they are acting on. + cfg.Environment = name + default: return Config{}, err } - var cfg Config - if err := json.Unmarshal(data, &cfg); err != nil { - return Config{}, fmt.Errorf("parsing %s: %w", path, err) - } return finishConfig(cfg, getenv, path) } diff --git a/internal/remote/remote_test.go b/internal/remote/remote_test.go index c5e4e13d..58989c1f 100644 --- a/internal/remote/remote_test.go +++ b/internal/remote/remote_test.go @@ -42,9 +42,11 @@ func stubAWSEnv(t *testing.T) { t.Setenv("AWS_EC2_METADATA_DISABLED", "true") } +// writeConfig registers the `default` environment, which is where a +// configuration lives now that no path outside the registry is read. func writeConfig(t *testing.T, cfg Config) { t.Helper() - path := must1(ConfigPath()) + path := must1(EnvConfigPath("default")) if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { t.Fatal(err) } @@ -63,13 +65,6 @@ func envMap(m map[string]string) func(string) string { return func(k string) string { return m[k] } } -func TestConfigPath(t *testing.T) { - t.Setenv("XDG_CONFIG_HOME", "/tmp/xdg") - if got, want := must1(ConfigPath()), "/tmp/xdg/spinloop/remote.json"; got != want { - t.Errorf("ConfigPath() = %q, want %q", got, want) - } -} - func TestLoadConfig_FromFile(t *testing.T) { isolateConfig(t) writeConfig(t, Config{ @@ -77,7 +72,7 @@ func TestLoadConfig_FromFile(t *testing.T) { StopURL: "https://stop.example/", Region: "eu-west-1", }) - cfg, err := LoadConfig(noEnv) + cfg, err := LoadEnvironment("default", noEnv) if err != nil { t.Fatal(err) } @@ -89,7 +84,7 @@ func TestLoadConfig_FromFile(t *testing.T) { func TestLoadConfig_EnvOverrides(t *testing.T) { isolateConfig(t) writeConfig(t, Config{StartURL: "https://old/", StopURL: "https://old-stop/", Region: "us-east-1"}) - cfg, err := LoadConfig(envMap(map[string]string{ + cfg, err := LoadEnvironment("default", envMap(map[string]string{ "SPINLOOP_REMOTE_START_URL": "https://new/", "SPINLOOP_REMOTE_REGION": "eu-west-2", })) @@ -113,7 +108,7 @@ func TestLoadConfig_RegionDerivedFromURL(t *testing.T) { StartURL: "https://abc123.lambda-url.eu-west-1.on.aws/", StopURL: "https://def456.lambda-url.eu-west-1.on.aws/", }) - cfg, err := LoadConfig(noEnv) + cfg, err := LoadEnvironment("default", noEnv) if err != nil { t.Fatal(err) } @@ -124,7 +119,7 @@ func TestLoadConfig_RegionDerivedFromURL(t *testing.T) { func TestLoadConfig_Unconfigured(t *testing.T) { isolateConfig(t) - _, err := LoadConfig(noEnv) + _, err := LoadEnvironment("default", noEnv) if err == nil || !strings.Contains(err.Error(), "not configured") { t.Errorf("expected a not-configured error, got %v", err) } @@ -133,7 +128,7 @@ func TestLoadConfig_Unconfigured(t *testing.T) { func TestLoadConfig_NoRegion(t *testing.T) { isolateConfig(t) writeConfig(t, Config{StartURL: "https://start.example/", StopURL: "https://stop.example/"}) - _, err := LoadConfig(noEnv) + _, err := LoadEnvironment("default", noEnv) if err == nil || !strings.Contains(err.Error(), "region") { t.Errorf("expected a region error, got %v", err) } @@ -1181,13 +1176,20 @@ func TestStart_ExpiredCredentials(t *testing.T) { } } -func TestLoadConfigFile(t *testing.T) { - path := filepath.Join(t.TempDir(), "remote.json") - content := `{"start_url":"https://start.example/","stop_url":"https://stop.example/","region":"eu-west-1"}` - if err := os.WriteFile(path, []byte(content), 0o600); err != nil { +// An environment resolves by name, from the registry, with the overrides +// applied on top of whatever the file holds. +func TestLoadEnvironment(t *testing.T) { + isolateConfig(t) + if err := SaveEnvironment("prod", Config{ + StartURL: "https://start.example/", + StopURL: "https://stop.example/", + Region: "eu-west-1", + Environment: "prod", + }); err != nil { t.Fatal(err) } - cfg, err := LoadConfigFile(path, noEnv) + + cfg, err := LoadEnvironment("prod", noEnv) if err != nil { t.Fatal(err) } @@ -1195,7 +1197,7 @@ func TestLoadConfigFile(t *testing.T) { t.Errorf("unexpected config: %+v", cfg) } - cfg, err = LoadConfigFile(path, envMap(map[string]string{"SPINLOOP_REMOTE_REGION": "eu-west-2"})) + cfg, err = LoadEnvironment("prod", envMap(map[string]string{"SPINLOOP_REMOTE_REGION": "eu-west-2"})) if err != nil { t.Fatal(err) } @@ -1204,10 +1206,16 @@ func TestLoadConfigFile(t *testing.T) { } } -func TestLoadConfigFile_Missing(t *testing.T) { - _, err := LoadConfigFile(filepath.Join(t.TempDir(), "remote.json"), noEnv) - if err == nil || !strings.Contains(err.Error(), "does not exist") { - t.Errorf("expected a does-not-exist error, got %v", err) +// A name with no file and no overrides fails naming the registry path to +// create, rather than a path the user never chose. +func TestLoadEnvironment_Missing(t *testing.T) { + isolateConfig(t) + _, err := LoadEnvironment("nosuchenv", noEnv) + if err == nil { + t.Fatal("expected a not-configured error") + } + if !strings.Contains(err.Error(), "remotes/nosuchenv/remote.json") { + t.Errorf("the failure should name the environment's registry path, got %v", err) } } @@ -1383,7 +1391,7 @@ func TestEnv_NoEnvURL(t *testing.T) { func TestLoadConfig_EnvURLOverride(t *testing.T) { isolateConfig(t) writeConfig(t, Config{StartURL: "https://start/", StopURL: "https://stop/", EnvURL: "https://old-env/", Region: "eu-west-1"}) - cfg, err := LoadConfig(envMap(map[string]string{ + cfg, err := LoadEnvironment("default", envMap(map[string]string{ "SPINLOOP_REMOTE_ENV_URL": "https://new-env/", })) if err != nil { diff --git a/internal/remote/seed_test.go b/internal/remote/seed_test.go index 27761eb1..fe71b0f2 100644 --- a/internal/remote/seed_test.go +++ b/internal/remote/seed_test.go @@ -296,7 +296,7 @@ func TestConfig_CarriesTheSeedURL(t *testing.T) { StartURL: "http://start", StopURL: "http://stop", SeedURL: "http://seed", Region: "eu-west-1", }) - cfg, err := LoadConfig(func(string) string { return "" }) + cfg, err := LoadEnvironment("default", func(string) string { return "" }) if err != nil { t.Fatal(err) } @@ -308,7 +308,7 @@ func TestConfig_CarriesTheSeedURL(t *testing.T) { func TestConfig_SeedURLOverride(t *testing.T) { isolateConfig(t) writeConfig(t, Config{StartURL: "http://start", StopURL: "http://stop", Region: "eu-west-1"}) - cfg, err := LoadConfig(func(k string) string { + cfg, err := LoadEnvironment("default", func(k string) string { if k == "SPINLOOP_REMOTE_SEED_URL" { return "http://override" } @@ -388,7 +388,7 @@ func TestControlPlaneFromOutputs_RejectsAStackMissingTheControlURLs(t *testing.T func TestConfig_LoadsWithoutASeedURL(t *testing.T) { isolateConfig(t) writeConfig(t, Config{StartURL: "http://start", StopURL: "http://stop", Region: "eu-west-1"}) - cfg, err := LoadConfig(func(string) string { return "" }) + cfg, err := LoadEnvironment("default", func(string) string { return "" }) if err != nil { t.Fatalf("a config without seed_url must still load: %v", err) } diff --git a/openspec/changes/require-named-environment/.openspec.yaml b/openspec/changes/require-named-environment/.openspec.yaml new file mode 100644 index 00000000..96db9a43 --- /dev/null +++ b/openspec/changes/require-named-environment/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-09-15 diff --git a/openspec/changes/require-named-environment/design.md b/openspec/changes/require-named-environment/design.md new file mode 100644 index 00000000..bfc15ab2 --- /dev/null +++ b/openspec/changes/require-named-environment/design.md @@ -0,0 +1,106 @@ +## Context + +See proposal.md — Why. The implementation-relevant current state: + +- `resolveRemoteConfig(envName, spinloopArg)` (`cmd/spinloop/remote.go`) reads + the Spinloop's `ENV` instructions, then branches: a named environment is + `EnvConfigPath` + `LoadConfigFile`; no name falls to `LoadDefault`. Nine + subcommands call it — `start`, `stop`, `pause`, `restart`, `status`, + `metrics`, `logs`, `env`, `keep`. `deploy` does not: it already requires the + flag. +- `LoadDefault` tries `remotes/default/remote.json`, then the superseded + `~/.config/spinloop/remote.json`, then falls through to + `finishConfig(Config{}, …)` — which is what makes an overrides-only + configuration work at all. +- `finishConfig` applies the `SPINLOOP_REMOTE_*` overrides and raises the + "remote is not configured: paste … into %s" failure, taking the source path + for the message. +- `Config.Environment` is set only by unmarshalling a file. Its own comment + says the shared Lambdas "reject a call without one", so an overrides-only + configuration reaches the control plane with no identifier. +- `LoadConfig` and `ConfigPath` exist but have no caller outside the test + suite once `LoadDefault`'s loop goes. + +## Goals / Non-Goals + +**Goals:** + +- No command changes the state of a cloud instance without being told which + one. +- One resolution rule for every environment name. +- The documented "no `remote.json` on disk" workflow keeps working, and starts + carrying the identifier it always needed. + +**Non-Goals:** + +- No stored "current environment" to make the flag optional again. That is the + implicit target under another name, and it is what this removes. If typing + the flag proves tiresome, the answer is a shell alias or a fleet file, both + of which already exist. +- No change to the registry layout, the fleet file, the control plane, or + `deploy`, which already requires the flag. + +## Decisions + +**D1: The flag is required on every subcommand, not only the mutating ones.** + +The danger is sharpest for `start`, `stop`, `pause`, `restart` and `keep`, and +a narrower change could require the flag only there. Against that: a rule with +an exception list is one an operator has to remember, and the reads are how the +mutations get typed — someone who runs `remote status` and then `remote stop` +in the same shell should not have the second mean something the first did not +warn them about. One rule, no list. + +Alternative: required for mutations, optional for reads (rejected — two rules, +and the read is the rehearsal for the write). + +**D2: `default` stays a legal name and loses only its privilege.** + +Removing the name as well as the fallback would be banning a string for no +safety gain: the risk was never the name, it was that a command assumed it. +`--env default` resolves like `--env prod`, and an existing `remotes/default/` +directory keeps working. + +**D3: A named environment with no file may be configured by the overrides, and +the name becomes the identifier.** + +The documented environment-variable workflow reached the control plane with an +empty `Config.Environment` — a latent bug, since the identifier is what selects +the instance. Requiring the flag supplies the missing piece for free: the name +the user typed is exactly the identifier that was absent. + +So `LoadConfigFile` takes the environment name, and where the file is absent +but the overrides are complete it returns a config carrying that name. This is +strictly more capable than today, in the one case that was quietly broken. + +Alternative: drop the overrides-only path with the default environment +(rejected — it is documented, it is how CI configures the commands, and the +bug it carried is fixed by this change rather than deepened by it). + +**D4: `LoadDefault`, `LoadConfig` and `ConfigPath` are deleted, not deprecated.** + +With no fallback there is no "the environment when none is named", so the +function that expressed it goes rather than lingering as a synonym for +`LoadConfigFile("default")`. `LoadConfig` and `ConfigPath` have no callers left. +Deleting rather than leaving them means a missed call site is a compile error. + +## Risks / Trade-offs + +- [A command that used to run bare now fails] → that is the change. The + failure names the flag and lists the registered environments, so the next + thing to type is in the error. +- [One environment still means typing the flag every time] → the cost of the + guarantee, and the reason D2 keeps the name short. A fleet file remains the + way to drive several targets without repeating yourself. +- [Requiring the flag on reads is stricter than the danger warrants] → D1's + trade, taken deliberately: one rule an operator learns once beats a list of + which commands are safe bare. +- [The overrides-only path changes shape] → it gains a required flag and a + correct identifier; the variables themselves are untouched, and a scenario + covers it. + +## Migration Plan + +None needed. A command that ran bare takes `--env `; a configuration at +the superseded path is moved into `remotes//` or re-created. Rollback is +a revert, and nothing is written, so no state needs undoing. diff --git a/openspec/changes/require-named-environment/proposal.md b/openspec/changes/require-named-environment/proposal.md new file mode 100644 index 00000000..fa492289 --- /dev/null +++ b/openspec/changes/require-named-environment/proposal.md @@ -0,0 +1,82 @@ +## Why + +`spinloop remote stop` with no flag stops something. Which instance depends on +what happens to be registered as `default` — a name the user may never have +chosen, left over from before they started naming environments. The same is +true of `start`, `pause`, `restart` and `keep`: five commands that change the +state of a cloud instance while naming no target. + +The danger is already on record. `remote deploy` requires `--env`, and the +change that made it so explains why: *"Creating an environment binds a name to +a machine; a silent default would hide that binding and risk clobbering the +`default` environment"*, rejecting an optional flag **as a footgun**. That +reasoning applies to stopping an instance as much as to deploying one; it was +simply applied to one command. + +Two further things fall out of the same root. The default environment resolves +differently from every other name — from the registry, *or* from a superseded +`~/.config/spinloop/remote.json`, with nothing saying which answered — so code +that resolves an environment by name has to special-case one name. And the +documented environment-variable workflow, which lets the remote commands run +with no `remote.json` at all, reaches the control plane with no environment +identifier: `Config.Environment` is set only by reading a file, and the +identifier is what tells the shared Lambdas which instance to act on. + +One change fixes all three, because they are the same thing: a target nobody +named. + +## What Changes + +- **BREAKING** Every `remote` subcommand requires `--env `. There is no + implicit target: a command that names no environment fails saying so and + listing the registered environments, rather than acting on one the user did + not choose. `deploy` already required it; the rest now match. +- **BREAKING** The `default` environment loses its special status. The name + stays valid — an environment may still be called `default` — but it is + resolved like any other name and is never assumed. +- **BREAKING** `~/.config/spinloop/remote.json` is no longer read, and its + reader goes with it. It was the second path only the default environment + consulted, and the reason resolving a name had a special case. +- The environment-variable workflow keeps working and starts carrying an + identifier: where `--env ` names an environment with no registered + file, a complete set of `SPINLOOP_REMOTE_*` overrides SHALL configure it, + and **the name given is the environment identifier** sent with each control + call. Today that path sends none. +- `remote.LoadDefault`, `remote.LoadConfig` and `remote.ConfigPath` are + removed. Nothing resolves an environment except by name. + +Fleet nodes are unaffected: a `kind: remote` node has always named its +environment by node name, which is why the fleet commands never had this +problem. + +## Capabilities + +### New Capabilities + +(None.) + +### Modified Capabilities + +- `remote-endpoint`: a `remote` subcommand names its environment explicitly; + there is no fallback when `--env` is absent, and a command without it fails + naming the registered environments. +- `remote-environments`: every environment resolves the same way, by name, + from the registry; a named environment with no file may be configured by the + `SPINLOOP_REMOTE_*` overrides, with its name as the identifier. +- `config-location`: the legacy `remote.json` is no longer one of the files + spinloop owns under its config directory. + +## Impact + +- `internal/remote`: `LoadDefault`, `LoadConfig` and `ConfigPath` deleted; + `LoadConfigFile` gains the name so it can supply the identifier when the + file is absent. +- `cmd/spinloop/remote.go`: `resolveRemoteConfig` loses its fallback branch and + requires the flag; every subcommand's `--env` is marked required. +- `internal/fleet`: nothing to change on main — the special case for the + default environment exists only on the unmerged top-level-verbs branch, and + is deleted there rather than carried. +- `docs/commands/remote.md`, `docs/env-vars.md`: the default environment stops + being described as a fallback, and the environment-variable workflow gains + the `--env` it always needed. +- No change to the registry layout, the control plane, or the fleet file. diff --git a/openspec/changes/require-named-environment/specs/config-location/spec.md b/openspec/changes/require-named-environment/specs/config-location/spec.md new file mode 100644 index 00000000..a25fa236 --- /dev/null +++ b/openspec/changes/require-named-environment/specs/config-location/spec.md @@ -0,0 +1,15 @@ +## MODIFIED Requirements + +### Requirement: Single resolved config directory + +spinloop SHALL resolve one config directory and place every file it owns under +it: its own `config.json` (default-harness preference and alias registry), the +`remotes//` environment registry, the daemon state directory, and the CDK +source directory. There SHALL be one resolver; the location SHALL NOT be +computed independently in more than one place. + +#### Scenario: All spinloop-owned state shares one root + +- **WHEN** the config directory resolves to a given path +- **THEN** `config.json`, the `remotes//` registry, and the daemon state + directory all resolve beneath that same path diff --git a/openspec/changes/require-named-environment/specs/remote-endpoint/spec.md b/openspec/changes/require-named-environment/specs/remote-endpoint/spec.md new file mode 100644 index 00000000..bba17a6f --- /dev/null +++ b/openspec/changes/require-named-environment/specs/remote-endpoint/spec.md @@ -0,0 +1,86 @@ +## MODIFIED Requirements + +### Requirement: Environment selection for remote commands + +The endpoint's control URLs SHALL come from a JSON configuration naming a start +URL, a stop URL, an optional deploy URL, and a region. That configuration MAY +also name the endpoint's own base URL; it SHALL be optional, since no control +call needs it, and a configuration without it SHALL remain valid. + +A `remote` subcommand SHALL select which environment's configuration it uses +with its `--env ` flag, and the flag SHALL be required: the value is a +registered environment's name, and the configuration is read from that +environment's `remote.json` in the per-user registry (see the Remote +Environments specification). A `--env` value that names an environment with no +registered configuration, and no complete configuration in the environment +variables, SHALL fail saying the environment is not registered and how to +create it. + +A subcommand given no `--env` SHALL fail naming the flag and listing the +registered environments, and SHALL act on nothing. There is no environment a +command falls back to: several of these subcommands change the state of a +cloud instance, and an instance nobody named is not one to start, stop or +terminate. The name `default` is an ordinary environment name, carrying no +special meaning. + +The Spinloop a subcommand is given as an argument SHALL NOT select an +environment; it SHALL be read only for its `ENV` instructions and the `.env` +file beside it, which the command applies before any AWS or control-plane work +(see the Remote Local Environment specification). + +Environment variables SHALL override individual values, and the region SHALL +fall back to the standard AWS region variable and then to the region named in +the URL. A missing or incomplete configuration SHALL fail saying where to put +it. + +#### Scenario: The flag selects the environment + +- **WHEN** the user runs `spinloop remote status --env qwen3.6-27b-prod` +- **THEN** the URLs come from that environment's `remote.json` in the registry + +#### Scenario: An unregistered environment is named as such + +- **WHEN** a `remote` subcommand runs with `--env missing` and no environment + `missing` is registered +- **THEN** it fails saying the environment is not registered and that + `spinloop remote deploy --env missing` creates it + +#### Scenario: No flag uses the default environment + +- **WHEN** a `remote` subcommand runs with no `--env` flag +- **THEN** the `default` environment is used, whether or not a `Spinloop` is + present in the working directory + +#### Scenario: An explicit Spinloop does not select an environment + +- **WHEN** a `remote` subcommand is given a Spinloop as its argument and no + `--env` flag +- **THEN** the command uses the `default` environment and applies the + Spinloop's `ENV` instructions and adjacent `.env` to the process environment, + rather than failing for the Spinloop to name an environment + +#### Scenario: Configuration without a base URL + +- **WHEN** a remote configuration names the control URLs and region but no base + URL, and a `remote` subcommand runs +- **THEN** the subcommand works as it always has, since the endpoint reports its + own address in the replies to `start` and `status` + +#### Scenario: A command with no environment names the flag + +- **WHEN** the user runs a `remote` subcommand with no `--env` +- **THEN** it fails naming `--env` and listing the registered environments, + and contacts nothing + +#### Scenario: An instance is never stopped without being named + +- **WHEN** the user runs `spinloop remote stop` with no `--env`, with an + environment named `default` registered +- **THEN** nothing is stopped: the command fails naming the flag, and + `default` is not assumed + +#### Scenario: default is an ordinary name + +- **WHEN** the user runs a `remote` subcommand with `--env default` and that + environment is registered +- **THEN** it acts on that environment, exactly as it would for any other name diff --git a/openspec/changes/require-named-environment/specs/remote-environments/spec.md b/openspec/changes/require-named-environment/specs/remote-environments/spec.md new file mode 100644 index 00000000..c5436cf8 --- /dev/null +++ b/openspec/changes/require-named-environment/specs/remote-environments/spec.md @@ -0,0 +1,43 @@ +## ADDED Requirements + +### Requirement: Every environment resolves by name + +An environment SHALL be resolved from its name and nothing else: the +configuration at `remotes//remote.json` in the per-user registry. No name +SHALL resolve by a different rule from any other, and no path outside the +registry SHALL be consulted, so a command that resolves an environment never +special-cases the name it was given. + +Where a named environment has no registered file, a complete set of +`SPINLOOP_REMOTE_*` overrides SHALL configure it, so the remote commands can +run with nothing on disk. In that case the **name given** SHALL be the +environment identifier sent with each control call — the identifier the shared +lifecycle Lambdas use to tell one instance from another, which a configuration +assembled from variables alone has no other source for. + +#### Scenario: A named environment reads its own file + +- **WHEN** a command runs with `--env prod` and `remotes/prod/remote.json` + exists +- **THEN** its configuration is read from that file + +#### Scenario: A file at the superseded path is not read + +- **WHEN** a command runs with `--env default`, no + `remotes/default/remote.json` exists, and a file exists at the path the + registry superseded +- **THEN** the command does not use that file's contents: no path outside the + registry is consulted for any name + +#### Scenario: Overrides configure a named environment with no file + +- **WHEN** a command runs with `--env ci`, no `remotes/ci/remote.json` exists, + and the `SPINLOOP_REMOTE_*` variables supply a complete configuration +- **THEN** the command works, and the control calls carry `ci` as the + environment identifier + +#### Scenario: Nothing anywhere fails naming the registry + +- **WHEN** no configuration can be assembled for the named environment +- **THEN** the failure says the environment is not registered and how to + create it diff --git a/openspec/changes/require-named-environment/tasks.md b/openspec/changes/require-named-environment/tasks.md new file mode 100644 index 00000000..f4bf54c4 --- /dev/null +++ b/openspec/changes/require-named-environment/tasks.md @@ -0,0 +1,44 @@ +## 1. One resolution rule + +- [x] 1.1 Give `LoadConfigFile` the environment name, and have it return a + config carrying that name as the identifier where the file is absent but + the `SPINLOOP_REMOTE_*` overrides are complete (design D3). Verify a test + that `--env ci` with no file and complete overrides yields a config whose + environment is `ci`. +- [x] 1.2 Delete `LoadDefault`, `LoadConfig` and `ConfigPath` (design D4). + Verify the tree builds — a missed caller is a compile error, not a + silent change. +- [x] 1.3 Verify no path outside the registry is consulted for any name: a + test that a file at the superseded path is not read for `--env default`. + +## 2. Requiring the flag + +- [x] 2.1 Make `--env` required on `start`, `stop`, `pause`, `restart`, + `status`, `metrics`, `logs`, `env` and `keep` (design D1), and drop + `resolveRemoteConfig`'s fallback branch so it takes a name it can rely + on. Verify each subcommand fails without the flag and works with it. +- [x] 2.2 Make the no-flag failure name `--env` and list the registered + environments, so the fix is visible from the error. Verify a test over a + registry holding two environments that both names appear. +- [x] 2.3 Verify a mutating command acts on nothing without the flag: a test + that `remote stop` with a registered `default` environment stops nothing + and fails naming the flag (the scenario the change exists for). +- [x] 2.4 Verify `--env default` still works exactly as any other name + (design D2), and that `deploy` — which already required the flag — is + unchanged. + +## 3. Consumers, docs and verification + +- [x] 3.1 Update the `internal/remote` tests: delete the ones whose subject is + the fallback or the legacy file, and move the ones that wrote either as + a fixture to the registry path, keeping what they assert. +- [x] 3.2 Update `cmd/spinloop` tests that invoke a remote subcommand bare, and + any example or script that does. Verify nothing invokes one without an + environment. +- [x] 3.3 Rewrite `docs/commands/remote.md`, where the default environment is + no longer a fallback, and `docs/env-vars.md`, where the no-`remote.json` + workflow gains the `--env` it always needed and the identifier it never + had. Neither mentions the superseded file as read. +- [x] 3.4 Run `gofmt -l .` (expect no output), `go vet ./...` and + `go test ./... -cover`, confirming total coverage is unchanged and still + >= 80%.