Enable db commands for externally-managed databases

app/app.go

@@ -1067,6 +1067,46 @@ func (a *App) GetECSEvents(service string) ([]ecstypes.ServiceEvent, error) {
 	return events, nil
 }
 
+// noDatabaseConfiguredError returns a targeted, actionable error for the case where
+// a DB command is run against an app that has no db utils engine configured
+// (Settings.DBUtils.Engine == ""). This replaces the unhelpful
+// "unknown database engine " message (empty engine) that used to surface here.
+//
+// It distinguishes two cases by checking for a DATABASE_URL config variable:
+//   - present: the app has an externally-managed database wired up via config,
+//     but db utils haven't been enabled for it -- point the user at
+//     `apppack modify app`, which infers the engine from DATABASE_URL.
+//   - absent: no database has been set up for the app at all -- point the user
+//     at `apppack create database`.
+func (a *App) noDatabaseConfiguredError() error {
+	// If we can't load config for whatever reason, fall back to the "no database
+	// configured" message -- it's the more common case and still actionable.
+	configVars, _ := a.GetConfig()
+
+	return noDatabaseConfiguredErrorFromConfig(a.Name, configVars)
+}
+
+// noDatabaseConfiguredErrorFromConfig is the pure, testable core of
+// noDatabaseConfiguredError: given the app's config variables, it returns the
+// targeted error message depending on whether DATABASE_URL is set.
+func noDatabaseConfiguredErrorFromConfig(appName string, configVars ConfigVariables) error {
+	for _, v := range configVars {
+		if v.Name == "DATABASE_URL" {
+			return fmt.Errorf(
+				"%s has a DATABASE_URL config variable set, but db utils are not enabled for it -- "+
+					"run `apppack modify app %s` to enable database commands",
+				appName, appName,
+			)
+		}
+	}
+
+	return fmt.Errorf(
+		"no database is configured for %s -- run `apppack create database` to create one, "+
+			"then attach it with `--addon-database`",
+		appName,
+	)
+}
+
 func (a *App) DBDumpLocation(prefix string) (*s3.GetObjectInput, error) {
 	currentTime := time.Now()
 
@@ -1088,6 +1128,8 @@ func (a *App) DBDumpLocation(prefix string) (*s3.GetObjectInput, error) {
 		extension = "sql.gz"
 	} else if strings.Contains(a.Settings.DBUtils.Engine, "postgres") {
 		extension = "dump"
+	} else if a.Settings.DBUtils.Engine == "" {
+		return nil, a.noDatabaseConfiguredError()
 	} else {
 		return nil, fmt.Errorf("unknown database engine %s", a.Settings.DBUtils.Engine)
 	}
@@ -1101,6 +1143,14 @@ func (a *App) DBDumpLocation(prefix string) (*s3.GetObjectInput, error) {
 }
 
 func (a *App) DBDumpLoadFamily() (*string, error) {
+	if err := a.LoadSettings(); err != nil {
+		return nil, err
+	}
+
+	if a.Settings.DBUtils.Engine == "" {
+		return nil, a.noDatabaseConfiguredError()
+	}
+
 	taskDefn, _, err := a.TaskDefinition("dbutils")
 	if err != nil {
 		return nil, err
@@ -1143,14 +1193,19 @@ func (a *App) DBShellTaskInfo() (*string, *string, error) {
 	var exec string
 
 	if strings.Contains(a.Settings.DBUtils.Engine, "mysql") {
-		database := a.Name
 		if a.IsReviewApp() {
-			database = fmt.Sprintf("%s-pr%s", database, *a.ReviewApp)
+			exec = "mysql --database=" + fmt.Sprintf("%s-pr%s", a.Name, *a.ReviewApp)
+		} else {
+			// No --database here: the db-utils image writes the database name
+			// parsed from DATABASE_URL into ~/.my.cnf, so a bare `mysql` resolves
+			// the right database whether it's a managed AppPack database or an
+			// externally-managed one (mirrors psql/~/.pg_service.conf below).
+			exec = "mysql"
 		}
-
-		exec = "mysql --database=" + database
 	} else if strings.Contains(a.Settings.DBUtils.Engine, "postgres") {
 		exec = "psql"
+	} else if a.Settings.DBUtils.Engine == "" {
+		return nil, nil, a.noDatabaseConfiguredError()
 	} else {
 		return nil, nil, fmt.Errorf("unknown database engine %s", a.Settings.DBUtils.Engine)
 	}

app/db_test.go

@@ -0,0 +1,65 @@
+package app
+
+import (
+	"strings"
+	"testing"
+)
+
+// TestNoDatabaseConfiguredErrorFromConfig covers both branches of the "no database
+// configured" helper: a DATABASE_URL config variable present (external database,
+// db utils not enabled) vs absent (no database at all).
+func TestNoDatabaseConfiguredErrorFromConfig(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name          string
+		configVars    ConfigVariables
+		wantErrSub    string
+		notWantErrSub string
+	}{
+		{
+			name:       "DATABASE_URL present -- points at modify app",
+			configVars: ConfigVariables{{Name: "DATABASE_URL", Value: "postgres://example"}},
+			wantErrSub: "apppack modify app myapp",
+		},
+		{
+			// The engine is inferred from DATABASE_URL, so there is no
+			// --external-database flag to advertise. Guard against a stale
+			// reference creeping back into user-facing text.
+			name:          "DATABASE_URL present -- does not advertise a removed flag",
+			configVars:    ConfigVariables{{Name: "DATABASE_URL", Value: "postgres://example"}},
+			wantErrSub:    "db utils are not enabled",
+			notWantErrSub: "--external-database",
+		},
+		{
+			name:       "DATABASE_URL absent -- points at create database",
+			configVars: ConfigVariables{{Name: "OTHER_VAR", Value: "foo"}},
+			wantErrSub: "apppack create database",
+		},
+		{
+			name:       "no config vars at all -- points at create database",
+			configVars: nil,
+			wantErrSub: "apppack create database",
+		},
+	}
+
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			err := noDatabaseConfiguredErrorFromConfig("myapp", tt.configVars)
+			if err == nil {
+				t.Fatal("expected an error, got nil")
+			}
+
+			if !strings.Contains(err.Error(), tt.wantErrSub) {
+				t.Errorf("error %q does not contain %q", err.Error(), tt.wantErrSub)
+			}
+
+			if tt.notWantErrSub != "" && strings.Contains(err.Error(), tt.notWantErrSub) {
+				t.Errorf("error %q should not contain %q", err.Error(), tt.notWantErrSub)
+			}
+		})
+	}
+}

app/shelltask_test.go

@@ -1,6 +1,7 @@
 package app
 
 import (
+	"strings"
 	"testing"
 
 	"github.com/aws/aws-sdk-go-v2/aws"
@@ -99,3 +100,88 @@ func TestShellTaskFamily(t *testing.T) {
 		t.Errorf("ShellTaskFamily() = %q, want %q", *family, "myapp-shell")
 	}
 }
+
+// TestDBShellTaskInfo covers the exec command DBShellTaskInfo builds per engine.
+// Settings is pre-populated in every case, which makes App.LoadSettings
+// short-circuit, so none of these touch AWS -- same technique as the tests in
+// cmd/config_test.go.
+func TestDBShellTaskInfo(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name       string
+		engine     string
+		reviewApp  *string
+		wantExec   string
+		wantErrSub string // non-empty: error must contain this substring
+	}{
+		{
+			// A managed AppPack database's Engine field is indistinguishable
+			// from an external one -- both are just "mysql" -- so a bare
+			// `mysql` must keep working for it too.
+			name:     "managed mysql gets a bare mysql",
+			engine:   "mysql",
+			wantExec: "mysql",
+		},
+		{
+			// The db-utils image resolves the database from DATABASE_URL via
+			// ~/.my.cnf, so no --database flag is needed (or correct) here.
+			name:     "external mysql gets a bare mysql",
+			engine:   "mysql",
+			wantExec: "mysql",
+		},
+		{
+			name:     "postgres is untouched by this change",
+			engine:   "postgres",
+			wantExec: "psql",
+		},
+		{
+			// Review apps cannot use external databases (the CloudFormation
+			// condition requires IsApp), so this path must stay byte-identical
+			// to its pre-existing behavior.
+			name:      "review app mysql keeps the explicit --database form",
+			engine:    "mysql",
+			reviewApp: aws.String("42"),
+			wantExec:  "mysql --database=myapp-pr42 myapp-pr42",
+		},
+		{
+			name:       "empty engine surfaces the no-database-configured error",
+			engine:     "",
+			wantErrSub: "no database is configured",
+		},
+	}
+
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			a := &App{Name: "myapp", ReviewApp: tt.reviewApp, Settings: &Settings{}}
+			a.Settings.DBUtils.Engine = tt.engine
+			a.Settings.DBUtils.ShellTaskFamily = "myapp-dbshell"
+
+			_, exec, err := a.DBShellTaskInfo()
+
+			if tt.wantErrSub != "" {
+				if err == nil {
+					t.Fatal("expected an error, got nil")
+				}
+				if !strings.Contains(err.Error(), tt.wantErrSub) {
+					t.Errorf("error %q does not contain %q", err.Error(), tt.wantErrSub)
+				}
+
+				return
+			}
+
+			if err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+			if exec == nil {
+				t.Fatal("exec = nil, want a command")
+			}
+			if *exec != tt.wantExec {
+				t.Errorf("exec = %q, want %q", *exec, tt.wantExec)
+			}
+		})
+	}
+}

cmd/config.go

@@ -34,6 +34,57 @@ import (
 	"github.com/spf13/cobra"
 )
 
+// databaseURLConfigVar is the config variable an externally-managed database is
+// reached through. Setting it is necessary but not sufficient: db utils also have
+// to be enabled on the app's stack before `apppack db ...` works.
+const databaseURLConfigVar = "DATABASE_URL"
+
+// hintEnableDBUtils prints a follow-up instruction after DATABASE_URL is stored on
+// an app whose db utils aren't enabled yet.
+//
+// Setting DATABASE_URL is the natural thing a user does when pointing an app at an
+// externally-managed database (Neon, Crunchy, etc.), but on its own it does nothing
+// for `apppack db shell`/`db dump`/`db load` -- those need the db-utils resources,
+// which only get created when the app's stack is updated. `apppack modify app` infers
+// the engine from this variable, so it's the whole of the remaining work; without a
+// nudge here there is nothing to tell the user that second step exists.
+//
+// This is advisory only. The config variable has already been stored successfully by
+// the time we're called, so every failure path is silent -- a hint is never worth
+// turning a succeeded command into a failed one.
+func hintEnableDBUtils(a *app.App) {
+	if !shouldHintEnableDBUtils(a) {
+		return
+	}
+
+	printWarning(fmt.Sprintf(
+		"db commands are not enabled for %s yet -- run `apppack modify app %s` to enable "+
+			"`apppack db shell`/`db dump`/`db load` against this database",
+		a.Name, a.Name,
+	))
+}
+
+// shouldHintEnableDBUtils is the decision half of hintEnableDBUtils, split out so the
+// branches are testable without stdout capture. A settings-load failure returns false:
+// we can't tell whether db utils are enabled, and guessing wrong means either nagging
+// a correctly-configured app or staying quiet on a misconfigured one -- silence is the
+// safer error for a purely advisory message.
+func shouldHintEnableDBUtils(a *app.App) bool {
+	// Review apps and pipelines can't use an external database (the CloudFormation
+	// condition requires IsApp), so the hint would be dead advice.
+	if a.IsReviewApp() || a.Pipeline {
+		return false
+	}
+
+	if err := a.LoadSettings(); err != nil {
+		return false
+	}
+
+	// A non-empty engine means db utils are already wired up, either by a managed
+	// AppPack database or by a previous `modify app` for this external one.
+	return a.Settings.DBUtils.Engine == ""
+}
+
 // configCmd represents the config command
 var configCmd = &cobra.Command{
 	Use:                   "config",
@@ -84,6 +135,10 @@ var setCmd = &cobra.Command{
 		checkErr(err)
 		ui.Spinner.Stop()
 		printSuccess("stored config variable " + name)
+
+		if name == databaseURLConfigVar {
+			hintEnableDBUtils(a)
+		}
 	},
 }
 
@@ -202,6 +257,8 @@ var configImportCmd = &cobra.Command{
 		checkErr(err)
 		imported := 0
 		skipped := 0
+		databaseURLImported := false
+
 		for key, val := range config {
 			err = a.SetConfig(key, val, importConfigOverride)
 			if err != nil {
@@ -216,6 +273,10 @@ var configImportCmd = &cobra.Command{
 				checkErr(err)
 			} else {
 				imported++
+
+				if key == databaseURLConfigVar {
+					databaseURLImported = true
+				}
 			}
 		}
 		msg := fmt.Sprintf("imported %d variables", imported)
@@ -224,6 +285,10 @@ var configImportCmd = &cobra.Command{
 		}
 		ui.Spinner.Stop()
 		printSuccess(msg)
+
+		if databaseURLImported {
+			hintEnableDBUtils(a)
+		}
 	},
 }