diff --git a/app/app.go b/app/app.go index af193a8..6b6b862 100644 --- a/app/app.go +++ b/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) } diff --git a/app/db_test.go b/app/db_test.go new file mode 100644 index 0000000..97b5a50 --- /dev/null +++ b/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) + } + }) + } +} diff --git a/app/shelltask_test.go b/app/shelltask_test.go index aaaa028..dfde027 100644 --- a/app/shelltask_test.go +++ b/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) + } + }) + } +} diff --git a/cmd/config.go b/cmd/config.go index 9724917..7e61fff 100644 --- a/cmd/config.go +++ b/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) + } }, } diff --git a/cmd/config_test.go b/cmd/config_test.go new file mode 100644 index 0000000..0761666 --- /dev/null +++ b/cmd/config_test.go @@ -0,0 +1,97 @@ +package cmd + +import ( + "testing" + + "github.com/apppackio/apppack/app" + "github.com/aws/aws-sdk-go-v2/aws" +) + +// TestShouldHintEnableDBUtils covers the decision behind the follow-up hint printed +// after DATABASE_URL is stored. Settings is pre-populated in every case, which makes +// App.LoadSettings short-circuit, so none of these touch AWS. +func TestShouldHintEnableDBUtils(t *testing.T) { + t.Parallel() + + settingsWithEngine := func(engine string) *app.Settings { + s := &app.Settings{} + s.DBUtils.Engine = engine + + return s + } + + tests := []struct { + name string + appName string + pipeline bool + reviewApp *string + settings *app.Settings + want bool + }{ + { + name: "plain app with no engine gets the hint", + appName: "myapp", + settings: settingsWithEngine(""), + want: true, + }, + { + // A managed AppPack database populates the engine, so the app already + // has working db commands and must not be nagged. + name: "engine already set (managed database) stays quiet", + appName: "myapp", + settings: settingsWithEngine("postgres"), + want: false, + }, + { + // Second `config set DATABASE_URL` after `modify app` already ran. + name: "engine already set (external database) stays quiet", + appName: "myapp", + settings: settingsWithEngine("mysql"), + want: false, + }, + { + // External databases are gated on IsApp in CloudFormation, so telling a + // pipeline to run `modify app` would be dead advice. + name: "pipeline stays quiet", + appName: "mypipeline", + pipeline: true, + settings: settingsWithEngine(""), + want: false, + }, + { + name: "review app stays quiet", + appName: "mypipeline", + reviewApp: aws.String("42"), + settings: settingsWithEngine(""), + want: false, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + a := &app.App{ + Name: tt.appName, + Pipeline: tt.pipeline, + ReviewApp: tt.reviewApp, + Settings: tt.settings, + } + + if got := shouldHintEnableDBUtils(a); got != tt.want { + t.Errorf("shouldHintEnableDBUtils() = %v, want %v", got, tt.want) + } + }) + } +} + +// TestDatabaseURLConfigVar pins the config variable name the hint keys off, since it +// has to match what the formations db-utils task definitions read from SSM. +func TestDatabaseURLConfigVar(t *testing.T) { + t.Parallel() + + if databaseURLConfigVar != "DATABASE_URL" { + t.Errorf("databaseURLConfigVar = %q, want %q", databaseURLConfigVar, "DATABASE_URL") + } +} diff --git a/stacks/app_pipeline.go b/stacks/app_pipeline.go index e5ea068..eaf6549 100644 --- a/stacks/app_pipeline.go +++ b/stacks/app_pipeline.go @@ -5,10 +5,12 @@ import ( "errors" "fmt" "math/rand" + "net/url" "os" "sort" "strings" + "github.com/apppackio/apppack/app" "github.com/apppackio/apppack/auth" "github.com/apppackio/apppack/bridge" "github.com/apppackio/apppack/ddb" @@ -49,12 +51,20 @@ type AppStackParameters struct { DatabaseStackName string `flag:"addon-database-name;fmtString:apppack-database-%s"` RedisAddonEnabled bool `flag:"addon-redis" cfnignore:"-"` RedisStackName string `flag:"addon-redis-name;fmtString:apppack-redis-%s"` - SQSQueueEnabled bool `flag:"addon-sqs"` - RepositoryType string - Fargate bool `flag:"ec2;negate"` - AllowedUsers []string `flag:"users"` - BuildWebhook bool `flag:"disable-build-webhook;negate"` - CustomTaskPolicyARN string `cfnparam:"CustomTaskPolicyArn"` + // ExternalDatabaseEngine IS a real CloudFormation parameter (unlike DatabaseAddonEnabled / + // RedisAddonEnabled above): it drives the db-utils container image tag at template render + // time for apps using an externally-managed database (Neon, Crunchy, etc.) reachable via + // the DATABASE_URL config variable. Valid values: "", "postgres", "mysql". + // + // There is no user-facing flag for this -- it is not settable directly. It is inferred + // in SetInternalFields from the app's DATABASE_URL scheme; see detectExternalDatabaseEngine. + ExternalDatabaseEngine string + SQSQueueEnabled bool `flag:"addon-sqs"` + RepositoryType string + Fargate bool `flag:"ec2;negate"` + AllowedUsers []string `flag:"users"` + BuildWebhook bool `flag:"disable-build-webhook;negate"` + CustomTaskPolicyARN string `cfnparam:"CustomTaskPolicyArn"` } var DefaultAppStackParameters = AppStackParameters{ @@ -113,9 +123,118 @@ func (p *AppStackParameters) SetInternalFields(cfg aws.Config, name *string) err return err } + // Infer ExternalDatabaseEngine from DATABASE_URL now that DatabaseStackName is + // fully resolved. This must run after resolveAddonStacks so a bare --addon-database + // (resolved to a stack name above) correctly wins over any external DATABASE_URL. + p.detectExternalDatabaseEngine(cfg) + return nil } +const ( + externalDatabaseEnginePostgres = "postgres" + externalDatabaseEngineMySQL = "mysql" +) + +// externalDatabaseConfigPath returns the SSM path for the app's DATABASE_URL config +// variable. Review apps are out of scope for external databases (see +// detectExternalDatabaseEngine), so this always uses the plain app config path. +func externalDatabaseConfigPath(appName string) string { + return fmt.Sprintf("/apppack/apps/%s/config/DATABASE_URL", appName) +} + +// fetchDatabaseURL fetches the app's DATABASE_URL config variable. It is a +// package-level function variable so tests can simulate SSM success/failure +// without making real AWS calls -- the same seam style used elsewhere in this +// package for injecting testable behavior around AWS-backed lookups. +var fetchDatabaseURL = func(cfg aws.Config, appName string) (string, bool) { // skipcq: CRT-P0003 + param, err := app.SsmParameter(cfg, externalDatabaseConfigPath(appName)) + if err != nil || param == nil || param.Value == nil { + return "", false + } + + return *param.Value, true +} + +// engineFromDatabaseURL infers a CloudFormation-accepted ExternalDatabaseEngine value +// from a DATABASE_URL's scheme, compared case-insensitively and only against the +// scheme -- never by substring-matching the whole URL, since a password could +// legitimately contain a string like "mysql". +// +// Returns ("", "") for an empty or unparseable URL (nothing to report -- this is the +// expected case before an app has a DATABASE_URL at all). Returns ("", scheme) when the +// URL parses but the scheme is not one we recognize, so the caller can warn the user by +// name rather than silently doing nothing. +func engineFromDatabaseURL(rawURL string) (engine, unrecognizedScheme string) { + if rawURL == "" { + return "", "" + } + + parsed, err := url.Parse(rawURL) + if err != nil || parsed.Scheme == "" { + return "", "" + } + + scheme := strings.ToLower(parsed.Scheme) + + switch scheme { + case "postgres", "postgresql", "pgsql", "psql": + return externalDatabaseEnginePostgres, "" + case "mysql", "mysql2", "mariadb": + return externalDatabaseEngineMySQL, "" + default: + return "", scheme + } +} + +// detectExternalDatabaseEngine populates ExternalDatabaseEngine by inspecting the app's +// DATABASE_URL, so the user never has to name the engine themselves. It runs for both +// `create app` and `modify app` (via SetInternalFields), which makes `modify app` the +// natural way to turn external db-utils support on after the app's DATABASE_URL is set. +// +// Idempotency: running this repeatedly must converge. An app with a managed database +// stays "" forever; an app whose DATABASE_URL is later unset goes back to "". +// +// Every failure mode here is silent-and-safe: a `create app` run happens before the app +// (and its DATABASE_URL parameter) exists, so a lookup miss is the expected, common case +// -- never fatal, never noisy. The one exception is a recognized-but-unmappable scheme, +// where staying silent would be exactly the unhelpful-error problem #146 is about. +func (p *AppStackParameters) detectExternalDatabaseEngine(cfg aws.Config) { // skipcq: CRT-P0003 + // A managed AppPack database always wins -- the two are mutually exclusive, and + // there is no longer a user input to reject, so we just force the field off. + if p.DatabaseAddonEnabled || p.DatabaseStackName != "" { + p.ExternalDatabaseEngine = "" + + return + } + + // The CloudFormation condition is And(ExternalDatabaseEngine != "", IsApp), so this + // can never take effect for pipelines or review apps. + if p.Type != DefaultAppStackParameters.Type { + p.ExternalDatabaseEngine = "" + + return + } + + databaseURL, ok := fetchDatabaseURL(cfg, p.Name) + if !ok { + p.ExternalDatabaseEngine = "" + + return + } + + engine, unrecognizedScheme := engineFromDatabaseURL(databaseURL) + if engine == "" && unrecognizedScheme != "" { + ui.PrintWarning(fmt.Sprintf( + "could not determine a database engine from %s's DATABASE_URL scheme %q -- "+ + "db commands (`apppack db shell`/`apppack db dump`) will not be enabled for it", + p.Name, unrecognizedScheme, + )) + } + + p.ExternalDatabaseEngine = engine +} + // resolveAddonStacks auto-selects database/redis stack names when the boolean // --addon-database / --addon-redis flags are set but no explicit instance name // was provided via --addon-database-name / --addon-redis-name. This makes the diff --git a/stacks/app_pipeline_test.go b/stacks/app_pipeline_test.go index fe173c0..8e5619e 100644 --- a/stacks/app_pipeline_test.go +++ b/stacks/app_pipeline_test.go @@ -5,6 +5,7 @@ import ( "testing" "github.com/apppackio/apppack/ui/uitest" + "github.com/aws/aws-sdk-go-v2/aws" tea "github.com/charmbracelet/bubbletea" "github.com/charmbracelet/huh" ) @@ -674,3 +675,223 @@ func TestSelectRedisStack(t *testing.T) { }) } } + +// --- engineFromDatabaseURL --- + +func TestEngineFromDatabaseURL(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + rawURL string + wantEngine string + wantUnrecognizedScheme string + }{ + {name: "empty string", rawURL: ""}, + {name: "postgres", rawURL: "postgres://user:pass@host:5432/db", wantEngine: "postgres"}, + {name: "postgresql", rawURL: "postgresql://user:pass@host:5432/db", wantEngine: "postgres"}, + {name: "pgsql", rawURL: "pgsql://user:pass@host:5432/db", wantEngine: "postgres"}, + {name: "psql", rawURL: "psql://user:pass@host:5432/db", wantEngine: "postgres"}, + {name: "mysql", rawURL: "mysql://user:pass@host:3306/db", wantEngine: "mysql"}, + {name: "mysql2", rawURL: "mysql2://user:pass@host:3306/db", wantEngine: "mysql"}, + {name: "mariadb", rawURL: "mariadb://user:pass@host:3306/db", wantEngine: "mysql"}, + {name: "scheme comparison is case-insensitive", rawURL: "POSTGRES://user:pass@host/db", wantEngine: "postgres"}, + { + name: "unknown scheme", + rawURL: "mongodb://user:pass@host:27017/db", + wantUnrecognizedScheme: "mongodb", + }, + {name: "malformed URL (parse error)", rawURL: "://not-a-url"}, + {name: "malformed URL (no scheme)", rawURL: "not a url at all"}, + { + // Guards against substring matching: the whole URL contains "mysql" (in + // the password) but the scheme is postgres, so the result must be postgres. + name: "password contains mysql but scheme is postgres", + rawURL: "postgres://user:mysqlpassword123@host:5432/db", + wantEngine: "postgres", + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + engine, unrecognizedScheme := engineFromDatabaseURL(tt.rawURL) + + if engine != tt.wantEngine { + t.Errorf("engine = %q, want %q", engine, tt.wantEngine) + } + + if unrecognizedScheme != tt.wantUnrecognizedScheme { + t.Errorf("unrecognizedScheme = %q, want %q", unrecognizedScheme, tt.wantUnrecognizedScheme) + } + }) + } +} + +// --- detectExternalDatabaseEngine --- + +// TestDetectExternalDatabaseEngine covers the auto-detection algorithm in +// SetInternalFields. The SSM read is stubbed via the fetchDatabaseURL function +// variable so these run with no AWS calls. +func TestDetectExternalDatabaseEngine(t *testing.T) { + tests := []struct { + name string + params AppStackParameters + databaseURL string + fetchOK bool + wantEngine string + }{ + { + name: "managed database via DatabaseStackName forces empty even with a postgres DATABASE_URL", + params: AppStackParameters{Type: "app", DatabaseStackName: "apppack-database-mydb"}, + databaseURL: "postgres://user:pass@host/db", + fetchOK: true, + wantEngine: "", + }, + { + name: "managed database via DatabaseAddonEnabled forces empty even with a postgres DATABASE_URL", + params: AppStackParameters{Type: "app", DatabaseAddonEnabled: true}, + databaseURL: "postgres://user:pass@host/db", + fetchOK: true, + wantEngine: "", + }, + { + name: "pipeline forces empty even with a postgres DATABASE_URL", + params: AppStackParameters{Type: "pipeline"}, + databaseURL: "postgres://user:pass@host/db", + fetchOK: true, + wantEngine: "", + }, + { + name: "SSM lookup failure yields empty", + params: AppStackParameters{Type: "app"}, + fetchOK: false, + wantEngine: "", + }, + { + name: "postgres DATABASE_URL is detected for a plain app", + params: AppStackParameters{Type: "app"}, + databaseURL: "postgres://user:pass@host/db", + fetchOK: true, + wantEngine: "postgres", + }, + { + name: "mysql DATABASE_URL is detected for a plain app", + params: AppStackParameters{Type: "app"}, + databaseURL: "mysql://user:pass@host/db", + fetchOK: true, + wantEngine: "mysql", + }, + { + name: "unrecognized scheme yields empty (warning is side effect, not asserted here)", + params: AppStackParameters{Type: "app"}, + databaseURL: "mongodb://user:pass@host/db", + fetchOK: true, + wantEngine: "", + }, + { + name: "no DATABASE_URL set (app not yet created) yields empty, matching an unset app", + params: AppStackParameters{Type: "app"}, + fetchOK: true, + wantEngine: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + originalFetch := fetchDatabaseURL + defer func() { fetchDatabaseURL = originalFetch }() + + databaseURL := tt.databaseURL + fetchOK := tt.fetchOK + fetchDatabaseURL = func(_ aws.Config, _ string) (string, bool) { + return databaseURL, fetchOK + } + + p := tt.params + p.detectExternalDatabaseEngine(aws.Config{}) + + if p.ExternalDatabaseEngine != tt.wantEngine { + t.Errorf("ExternalDatabaseEngine = %q, want %q", p.ExternalDatabaseEngine, tt.wantEngine) + } + }) + } +} + +// TestDetectExternalDatabaseEngineIdempotent verifies that repeatedly running detection +// (as `apppack modify app` would on every invocation) converges rather than flip-flopping: +// a managed database stays "" forever, and an app whose DATABASE_URL is later unset goes +// back to "". +func TestDetectExternalDatabaseEngineIdempotent(t *testing.T) { + originalFetch := fetchDatabaseURL + defer func() { fetchDatabaseURL = originalFetch }() + + p := AppStackParameters{Type: "app"} + + // DATABASE_URL set to postgres -- engine detected. + fetchDatabaseURL = func(_ aws.Config, _ string) (string, bool) { + return "postgres://user:pass@host/db", true + } + + p.detectExternalDatabaseEngine(aws.Config{}) + + if p.ExternalDatabaseEngine != "postgres" { + t.Fatalf("ExternalDatabaseEngine = %q, want %q after first detection", p.ExternalDatabaseEngine, "postgres") + } + + // Running again with the same DATABASE_URL converges to the same value. + p.detectExternalDatabaseEngine(aws.Config{}) + + if p.ExternalDatabaseEngine != "postgres" { + t.Fatalf("ExternalDatabaseEngine = %q, want %q to stay stable on repeat", p.ExternalDatabaseEngine, "postgres") + } + + // DATABASE_URL is later unset -- detection must go back to "". + fetchDatabaseURL = func(_ aws.Config, _ string) (string, bool) { + return "", false + } + + p.detectExternalDatabaseEngine(aws.Config{}) + + if p.ExternalDatabaseEngine != "" { + t.Errorf("ExternalDatabaseEngine = %q, want empty after DATABASE_URL is unset", p.ExternalDatabaseEngine) + } +} + +// TestExternalDatabaseEngineRoundTrip verifies ExternalDatabaseEngine is a real +// CloudFormation parameter (no cfnignore) and round-trips through +// ToCloudFormationParameters / Import. +func TestExternalDatabaseEngineRoundTrip(t *testing.T) { + t.Helper() + + params := AppStackParameters{ExternalDatabaseEngine: "postgres"} + + cfnParams, err := params.ToCloudFormationParameters() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + var found bool + for _, p := range cfnParams { + if *p.ParameterKey == "ExternalDatabaseEngine" { + found = true + if *p.ParameterValue != "postgres" { + t.Errorf("ParameterValue = %q, want %q", *p.ParameterValue, "postgres") + } + } + } + if !found { + t.Fatal("ExternalDatabaseEngine parameter not found in CloudFormation parameters") + } + + var imported AppStackParameters + if err := imported.Import(cfnParams); err != nil { + t.Fatalf("unexpected error importing: %v", err) + } + + if imported.ExternalDatabaseEngine != "postgres" { + t.Errorf("imported ExternalDatabaseEngine = %q, want %q", imported.ExternalDatabaseEngine, "postgres") + } +}