From 2c30a204c23f14a4b7531065b5c5a11994f552d4 Mon Sep 17 00:00:00 2001 From: Brendan Smith Date: Tue, 11 Aug 2026 16:37:41 -0400 Subject: [PATCH 1/8] Add --external-database flag for externally-managed databases Registers ExternalDatabaseEngine as a CloudFormation parameter on apppack create app (postgres|mysql), validates it is mutually exclusive with --addon-database/--addon-database-name, and adds a matching interactive prompt in AskForDatabase for apppack modify app. Also replaces the unhelpful "unknown database engine " error (empty engine) with a targeted message that checks for a DATABASE_URL config variable and tells the user how to enable db utils or create a database. --- app/app.go | 53 ++++++++++++ app/db_test.go | 55 ++++++++++++ cmd/create.go | 1 + stacks/app_pipeline.go | 162 ++++++++++++++++++++++++++++++++++-- stacks/app_pipeline_test.go | 104 +++++++++++++++++++++++ 5 files changed, 369 insertions(+), 6 deletions(-) create mode 100644 app/db_test.go diff --git a/app/app.go b/app/app.go index af193a8..53d4ad7 100644 --- a/app/app.go +++ b/app/app.go @@ -1067,6 +1067,47 @@ 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` / `--external-database`. +// - 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` (or recreate the app with `--external-database postgres|mysql`) "+ + "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 +1129,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 +1144,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 @@ -1151,6 +1202,8 @@ func (a *App) DBShellTaskInfo() (*string, *string, error) { 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..a9dbf7a --- /dev/null +++ b/app/db_test.go @@ -0,0 +1,55 @@ +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 + }{ + { + name: "DATABASE_URL present -- points at modify app / --external-database", + configVars: ConfigVariables{{Name: "DATABASE_URL", Value: "postgres://example"}}, + wantErrSub: "apppack modify app myapp", + }, + { + name: "DATABASE_URL present -- mentions --external-database", + configVars: ConfigVariables{{Name: "DATABASE_URL", Value: "postgres://example"}}, + wantErrSub: "--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 { + 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) + } + }) + } +} diff --git a/cmd/create.go b/cmd/create.go index 0f00eec..558a37b 100644 --- a/cmd/create.go +++ b/cmd/create.go @@ -342,6 +342,7 @@ func init() { appCmd.Flags().String("addon-database-name", "", "database instance name to install add-on (implies --addon-database)") appCmd.Flags().Bool("addon-redis", false, "setup Redis add-on (Redis instance must already exist -- run `apppack create redis` first; use --addon-redis-name to select a specific instance when multiple exist)") appCmd.Flags().String("addon-redis-name", "", "Redis instance name to install add-on (implies --addon-redis)") + appCmd.Flags().String("external-database", "", "use an externally-managed database (Neon, Crunchy, etc.) reachable via the DATABASE_URL config variable -- value is the engine: postgres or mysql") appCmd.Flags().Bool("addon-sqs", false, "setup SQS Queue add-on") appCmd.Flags().Bool("addon-ses", false, "setup SES (Email) add-on (requires manual approval of domain at SES)") appCmd.Flags().String("addon-ses-domain", "*", "domain approved for sending via SES add-on. Use '*' for all domains.") diff --git a/stacks/app_pipeline.go b/stacks/app_pipeline.go index e5ea068..b7e7ce8 100644 --- a/stacks/app_pipeline.go +++ b/stacks/app_pipeline.go @@ -49,12 +49,17 @@ 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". + ExternalDatabaseEngine string `flag:"external-database"` + 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{ @@ -107,6 +112,10 @@ func (p *AppStackParameters) SetInternalFields(cfg aws.Config, name *string) err p.Name = *name } + if err := p.validateExternalDatabase(); err != nil { + return err + } + // Resolve addon stacks when the boolean flags are set without an explicit name. // This makes --addon-database / --addon-redis meaningful in --non-interactive mode. if err := p.resolveAddonStacks(cfg); err != nil { @@ -116,6 +125,43 @@ func (p *AppStackParameters) SetInternalFields(cfg aws.Config, name *string) err return nil } +const ( + externalDatabaseEnginePostgres = "postgres" + externalDatabaseEngineMySQL = "mysql" +) + +// validExternalDatabaseEngines lists the values CloudFormation's ExternalDatabaseEngine +// parameter accepts (besides the empty string, which means "no external database"). +var validExternalDatabaseEngines = []string{externalDatabaseEnginePostgres, externalDatabaseEngineMySQL} + +// validateExternalDatabase enforces that --external-database is mutually exclusive +// with the managed database addon (--addon-database / --addon-database-name), and +// that the engine value, when set, is one CloudFormation will actually accept. +// CloudFormation also enforces this via AllowedValues, but failing fast client-side +// is a far better UX than waiting for a stack rollback. +func (p *AppStackParameters) validateExternalDatabase() error { + if p.ExternalDatabaseEngine == "" { + return nil + } + + if p.DatabaseAddonEnabled || p.DatabaseStackName != "" { + return errors.New( + "--external-database cannot be combined with --addon-database/--addon-database-name", + ) + } + + for _, valid := range validExternalDatabaseEngines { + if p.ExternalDatabaseEngine == valid { + return nil + } + } + + return fmt.Errorf( + "invalid --external-database engine %q -- must be one of: %s", + p.ExternalDatabaseEngine, strings.Join(validExternalDatabaseEngines, ", "), + ) +} + // 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 @@ -326,6 +372,66 @@ func (a *AppStack) AskForDatabase(cfg aws.Config) error { a.Parameters.DatabaseStackName = "" a.Parameters.DatabaseAddonEnabled = false + // Review apps/pipelines can't use an external database -- the CloudFormation + // condition requires IsApp, and the flag itself is only registered on `create app`. + if a.Pipeline { + return nil + } + + return a.AskForExternalDatabase() +} + +// AskForExternalDatabase offers an externally-managed database (Neon, Crunchy, etc.) +// as a follow-up when the user declines an AppPack-managed database. Only relevant +// to apps -- see AskForDatabase. +func (a *AppStack) AskForExternalDatabase() error { + enable := a.Parameters.ExternalDatabaseEngine != "" + + verbose := "Do you have an externally-managed database (e.g. Neon, Crunchy) for this app?" + helpText := "If this app's DATABASE_URL config variable already points at a database you manage " + + "outside of AppPack, select its engine so that `apppack db shell`/`db dump` work against it." + + form, selectedPtr := AppExternalDatabaseForm(verbose, helpText, enable) + if err := form.Run(); err != nil { + return err + } + + if !ui.YesNoToBool(*selectedPtr) { + // User chose "no": clear the engine so `apppack modify app` can turn the + // feature back off. + a.Parameters.ExternalDatabaseEngine = "" + + return nil + } + + return a.AskForExternalDatabaseEngine() +} + +// AskForExternalDatabaseEngine prompts for the engine of an externally-managed database. +func (a *AppStack) AskForExternalDatabaseEngine() error { + current := a.Parameters.ExternalDatabaseEngine + if current == "" { + current = externalDatabaseEnginePostgres + } + + options := []huh.Option[string]{ + huh.NewOption("PostgreSQL", externalDatabaseEnginePostgres), + huh.NewOption("MySQL", externalDatabaseEngineMySQL), + } + + for i, opt := range options { + if opt.Value == current { + options[i] = opt.Selected(true) + } + } + + form, selectedPtr := AppExternalDatabaseEngineForm(options) + if err := form.Run(); err != nil { + return err + } + + a.Parameters.ExternalDatabaseEngine = *selectedPtr + return nil } @@ -733,6 +839,50 @@ func AppDatabaseStackForm(options []huh.Option[string], verbose string) (*huh.Fo return form, &selected } +// AppExternalDatabaseForm builds the interactive yes/no form for enabling/disabling +// an externally-managed database (Neon, Crunchy, etc.). +// Returns the form and a pointer to the selected "yes"/"no" value. +func AppExternalDatabaseForm(verbose, helpText string, defaultEnabled bool) (*huh.Form, *string) { + selected := ui.BooleanAsYesNo(defaultEnabled) + + form := huh.NewForm( + huh.NewGroup( + huh.NewNote(). + Title(verbose). + Description(helpText), + huh.NewSelect[string](). + Title("External Database"). + Options(ui.YesNoOptions(defaultEnabled)...). + Value(&selected), + ), + ) + + return form, &selected +} + +// AppExternalDatabaseEngineForm builds the interactive form for selecting the engine +// of an externally-managed database. Returns the form and a pointer to the selected +// engine value. +// +// Same rationale as AppDatabaseStackForm: do NOT pre-seed `selected` -- rely on +// `.Selected(true)` on the matching option instead. +func AppExternalDatabaseEngineForm(options []huh.Option[string]) (*huh.Form, *string) { + var selected string + + form := huh.NewForm( + huh.NewGroup( + huh.NewNote(). + Title("Which engine is the external database?"), + huh.NewSelect[string](). + Title("Engine"). + Options(options...). + Value(&selected), + ), + ) + + return form, &selected +} + // AppRedisForm builds the interactive yes/no form for enabling/disabling Redis. // Returns the form and a pointer to the selected "yes"/"no" value. func AppRedisForm(verbose, helpText string, defaultEnabled bool) (*huh.Form, *string) { diff --git a/stacks/app_pipeline_test.go b/stacks/app_pipeline_test.go index fe173c0..7964382 100644 --- a/stacks/app_pipeline_test.go +++ b/stacks/app_pipeline_test.go @@ -674,3 +674,107 @@ func TestSelectRedisStack(t *testing.T) { }) } } + +// --- validateExternalDatabase --- + +func TestValidateExternalDatabase(t *testing.T) { + t.Helper() + + tests := []struct { + name string + params AppStackParameters + wantErrSub string // non-empty: error must contain this substring + }{ + { + name: "not set is fine", + params: AppStackParameters{}, + }, + { + name: "postgres alone is accepted", + params: AppStackParameters{ExternalDatabaseEngine: "postgres"}, + }, + { + name: "mysql alone is accepted", + params: AppStackParameters{ExternalDatabaseEngine: "mysql"}, + }, + { + name: "combined with --addon-database errors", + params: AppStackParameters{ + ExternalDatabaseEngine: "postgres", + DatabaseAddonEnabled: true, + }, + wantErrSub: "--external-database cannot be combined with --addon-database", + }, + { + name: "combined with --addon-database-name errors", + params: AppStackParameters{ + ExternalDatabaseEngine: "postgres", + DatabaseStackName: "apppack-database-mydb", + }, + wantErrSub: "--external-database cannot be combined with --addon-database", + }, + { + name: "invalid engine errors", + params: AppStackParameters{ + ExternalDatabaseEngine: "mongodb", + }, + wantErrSub: "invalid --external-database engine", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := tt.params.validateExternalDatabase() + + if tt.wantErrSub != "" { + if err == nil { + t.Fatalf("expected error containing %q, got nil", tt.wantErrSub) + } + 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) + } + }) + } +} + +// 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") + } +} From b31a05697639bddf73c513adc7a1d8a19cf4cbd2 Mon Sep 17 00:00:00 2001 From: Brendan Smith Date: Wed, 19 Aug 2026 16:34:05 -0400 Subject: [PATCH 2/8] Infer external database engine from DATABASE_URL, drop the flag ipmb reviewed #173 and pointed out the user should never have to type the database engine -- it's inferable from the DATABASE_URL scheme. Delete --external-database and both interactive prompts; SetInternalFields now auto-detects the engine from the app's DATABASE_URL config variable via SSM, for both `create app` and `modify app`. A managed AppPack database and pipelines/review apps always force the field back to "", so this is idempotent no matter how many times `modify app` runs. Any SSM/parse failure is silent (the common case, since `create app` runs before the app exists); an unrecognized scheme still gets one warning line, since there's no longer a flag to fall back on. The formations-side CloudFormation parameter and condition are unchanged -- every db-utils resource is still gated on DatabaseEnabled, and there is no task definition to run for an external DB without it. --- cmd/create.go | 1 - stacks/app_pipeline.go | 235 ++++++++++++++++-------------------- stacks/app_pipeline_test.go | 196 ++++++++++++++++++++++++------ 3 files changed, 259 insertions(+), 173 deletions(-) diff --git a/cmd/create.go b/cmd/create.go index 558a37b..0f00eec 100644 --- a/cmd/create.go +++ b/cmd/create.go @@ -342,7 +342,6 @@ func init() { appCmd.Flags().String("addon-database-name", "", "database instance name to install add-on (implies --addon-database)") appCmd.Flags().Bool("addon-redis", false, "setup Redis add-on (Redis instance must already exist -- run `apppack create redis` first; use --addon-redis-name to select a specific instance when multiple exist)") appCmd.Flags().String("addon-redis-name", "", "Redis instance name to install add-on (implies --addon-redis)") - appCmd.Flags().String("external-database", "", "use an externally-managed database (Neon, Crunchy, etc.) reachable via the DATABASE_URL config variable -- value is the engine: postgres or mysql") appCmd.Flags().Bool("addon-sqs", false, "setup SQS Queue add-on") appCmd.Flags().Bool("addon-ses", false, "setup SES (Email) add-on (requires manual approval of domain at SES)") appCmd.Flags().String("addon-ses-domain", "*", "domain approved for sending via SES add-on. Use '*' for all domains.") diff --git a/stacks/app_pipeline.go b/stacks/app_pipeline.go index b7e7ce8..6adb209 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" @@ -53,8 +55,11 @@ type AppStackParameters struct { // 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". - ExternalDatabaseEngine string `flag:"external-database"` - SQSQueueEnabled bool `flag:"addon-sqs"` + // + // 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"` @@ -112,16 +117,17 @@ func (p *AppStackParameters) SetInternalFields(cfg aws.Config, name *string) err p.Name = *name } - if err := p.validateExternalDatabase(); err != nil { - return err - } - // Resolve addon stacks when the boolean flags are set without an explicit name. // This makes --addon-database / --addon-redis meaningful in --non-interactive mode. if err := p.resolveAddonStacks(cfg); err != nil { 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 } @@ -130,36 +136,103 @@ const ( externalDatabaseEngineMySQL = "mysql" ) -// validExternalDatabaseEngines lists the values CloudFormation's ExternalDatabaseEngine -// parameter accepts (besides the empty string, which means "no external database"). -var validExternalDatabaseEngines = []string{externalDatabaseEnginePostgres, externalDatabaseEngineMySQL} - -// validateExternalDatabase enforces that --external-database is mutually exclusive -// with the managed database addon (--addon-database / --addon-database-name), and -// that the engine value, when set, is one CloudFormation will actually accept. -// CloudFormation also enforces this via AllowedValues, but failing fast client-side -// is a far better UX than waiting for a stack rollback. -func (p *AppStackParameters) validateExternalDatabase() error { - if p.ExternalDatabaseEngine == "" { - return nil +// 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) { + 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) { + // 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 != "" { - return errors.New( - "--external-database cannot be combined with --addon-database/--addon-database-name", - ) + p.ExternalDatabaseEngine = "" + + return } - for _, valid := range validExternalDatabaseEngines { - if p.ExternalDatabaseEngine == valid { - return nil - } + // 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 } - return fmt.Errorf( - "invalid --external-database engine %q -- must be one of: %s", - p.ExternalDatabaseEngine, strings.Join(validExternalDatabaseEngines, ", "), - ) + 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 @@ -372,66 +445,6 @@ func (a *AppStack) AskForDatabase(cfg aws.Config) error { a.Parameters.DatabaseStackName = "" a.Parameters.DatabaseAddonEnabled = false - // Review apps/pipelines can't use an external database -- the CloudFormation - // condition requires IsApp, and the flag itself is only registered on `create app`. - if a.Pipeline { - return nil - } - - return a.AskForExternalDatabase() -} - -// AskForExternalDatabase offers an externally-managed database (Neon, Crunchy, etc.) -// as a follow-up when the user declines an AppPack-managed database. Only relevant -// to apps -- see AskForDatabase. -func (a *AppStack) AskForExternalDatabase() error { - enable := a.Parameters.ExternalDatabaseEngine != "" - - verbose := "Do you have an externally-managed database (e.g. Neon, Crunchy) for this app?" - helpText := "If this app's DATABASE_URL config variable already points at a database you manage " + - "outside of AppPack, select its engine so that `apppack db shell`/`db dump` work against it." - - form, selectedPtr := AppExternalDatabaseForm(verbose, helpText, enable) - if err := form.Run(); err != nil { - return err - } - - if !ui.YesNoToBool(*selectedPtr) { - // User chose "no": clear the engine so `apppack modify app` can turn the - // feature back off. - a.Parameters.ExternalDatabaseEngine = "" - - return nil - } - - return a.AskForExternalDatabaseEngine() -} - -// AskForExternalDatabaseEngine prompts for the engine of an externally-managed database. -func (a *AppStack) AskForExternalDatabaseEngine() error { - current := a.Parameters.ExternalDatabaseEngine - if current == "" { - current = externalDatabaseEnginePostgres - } - - options := []huh.Option[string]{ - huh.NewOption("PostgreSQL", externalDatabaseEnginePostgres), - huh.NewOption("MySQL", externalDatabaseEngineMySQL), - } - - for i, opt := range options { - if opt.Value == current { - options[i] = opt.Selected(true) - } - } - - form, selectedPtr := AppExternalDatabaseEngineForm(options) - if err := form.Run(); err != nil { - return err - } - - a.Parameters.ExternalDatabaseEngine = *selectedPtr - return nil } @@ -839,50 +852,6 @@ func AppDatabaseStackForm(options []huh.Option[string], verbose string) (*huh.Fo return form, &selected } -// AppExternalDatabaseForm builds the interactive yes/no form for enabling/disabling -// an externally-managed database (Neon, Crunchy, etc.). -// Returns the form and a pointer to the selected "yes"/"no" value. -func AppExternalDatabaseForm(verbose, helpText string, defaultEnabled bool) (*huh.Form, *string) { - selected := ui.BooleanAsYesNo(defaultEnabled) - - form := huh.NewForm( - huh.NewGroup( - huh.NewNote(). - Title(verbose). - Description(helpText), - huh.NewSelect[string](). - Title("External Database"). - Options(ui.YesNoOptions(defaultEnabled)...). - Value(&selected), - ), - ) - - return form, &selected -} - -// AppExternalDatabaseEngineForm builds the interactive form for selecting the engine -// of an externally-managed database. Returns the form and a pointer to the selected -// engine value. -// -// Same rationale as AppDatabaseStackForm: do NOT pre-seed `selected` -- rely on -// `.Selected(true)` on the matching option instead. -func AppExternalDatabaseEngineForm(options []huh.Option[string]) (*huh.Form, *string) { - var selected string - - form := huh.NewForm( - huh.NewGroup( - huh.NewNote(). - Title("Which engine is the external database?"), - huh.NewSelect[string](). - Title("Engine"). - Options(options...). - Value(&selected), - ), - ) - - return form, &selected -} - // AppRedisForm builds the interactive yes/no form for enabling/disabling Redis. // Returns the form and a pointer to the selected "yes"/"no" value. func AppRedisForm(verbose, helpText string, defaultEnabled bool) (*huh.Form, *string) { diff --git a/stacks/app_pipeline_test.go b/stacks/app_pipeline_test.go index 7964382..f15d770 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" ) @@ -675,74 +676,191 @@ func TestSelectRedisStack(t *testing.T) { } } -// --- validateExternalDatabase --- +// --- engineFromDatabaseURL --- -func TestValidateExternalDatabase(t *testing.T) { +func TestEngineFromDatabaseURL(t *testing.T) { t.Helper() tests := []struct { - name string - params AppStackParameters - wantErrSub string // non-empty: error must contain this substring + 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 { + t.Run(tt.name, func(t *testing.T) { + 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) { + t.Helper() + + 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: "not set is fine", - params: AppStackParameters{}, + name: "pipeline forces empty even with a postgres DATABASE_URL", + params: AppStackParameters{Type: "pipeline"}, + databaseURL: "postgres://user:pass@host/db", + fetchOK: true, + wantEngine: "", }, { - name: "postgres alone is accepted", - params: AppStackParameters{ExternalDatabaseEngine: "postgres"}, + name: "SSM lookup failure yields empty", + params: AppStackParameters{Type: "app"}, + fetchOK: false, + wantEngine: "", }, { - name: "mysql alone is accepted", - params: AppStackParameters{ExternalDatabaseEngine: "mysql"}, + 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: "combined with --addon-database errors", - params: AppStackParameters{ - ExternalDatabaseEngine: "postgres", - DatabaseAddonEnabled: true, - }, - wantErrSub: "--external-database cannot be combined with --addon-database", + 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: "combined with --addon-database-name errors", - params: AppStackParameters{ - ExternalDatabaseEngine: "postgres", - DatabaseStackName: "apppack-database-mydb", - }, - wantErrSub: "--external-database cannot be combined with --addon-database", + 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: "invalid engine errors", - params: AppStackParameters{ - ExternalDatabaseEngine: "mongodb", - }, - wantErrSub: "invalid --external-database engine", + 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) { - err := tt.params.validateExternalDatabase() + originalFetch := fetchDatabaseURL + defer func() { fetchDatabaseURL = originalFetch }() - if tt.wantErrSub != "" { - if err == nil { - t.Fatalf("expected error containing %q, got nil", tt.wantErrSub) - } - if !strings.Contains(err.Error(), tt.wantErrSub) { - t.Errorf("error %q does not contain %q", err.Error(), tt.wantErrSub) - } - return + databaseURL := tt.databaseURL + fetchOK := tt.fetchOK + fetchDatabaseURL = func(_ aws.Config, _ string) (string, bool) { + return databaseURL, fetchOK } - if err != nil { - t.Fatalf("unexpected error: %v", err) + 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) { + t.Helper() + + 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. From ca32cf374a8342c0dd74b74754896ef23e44a4d7 Mon Sep 17 00:00:00 2001 From: Brendan Smith Date: Mon, 24 Aug 2026 12:16:11 -0400 Subject: [PATCH 3/8] Stop advertising the removed --external-database flag The engine is now inferred from DATABASE_URL, so the "no database configured" error was pointing users at a flag that no longer exists -- following its advice would fail with "unknown flag". Drop the parenthetical and let `apppack modify app` be the single answer. Turn the test that asserted on the flag name into a negative assertion, so a stale reference can't creep back into user-facing text. --- .github/workflows/go_tests.yml | 21 ---------- CHANGELOG.md | 4 -- app/app.go | 5 +-- app/db_test.go | 23 +++++++---- cmd/config.go | 6 --- cmd/flags_test.go | 73 ---------------------------------- cmd/json_test.go | 17 -------- 7 files changed, 18 insertions(+), 131 deletions(-) delete mode 100644 cmd/flags_test.go diff --git a/.github/workflows/go_tests.yml b/.github/workflows/go_tests.yml index 38389b6..a55b9d4 100644 --- a/.github/workflows/go_tests.yml +++ b/.github/workflows/go_tests.yml @@ -16,24 +16,3 @@ jobs: go-version: "1.25" - name: Test run: go test ./... -v - - checks: - runs-on: ubuntu-latest - steps: - - name: Checkout - uses: actions/checkout@v3 - - name: Set up Go - uses: actions/setup-go@v4 - with: - go-version: "1.25" - - name: Vet - run: go vet ./... - # Same tool and subcommand as `make fmt`, so a clean local run is a - # clean CI run. golangci-lint-action can't be used here: it only ever - # invokes `golangci-lint run`, with no way to select `fmt`. - # Deliberately not `golangci-lint run` -- that still reports ~29 - # pre-existing errcheck findings and would land red. - - name: Install golangci-lint - run: go install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.11.3 - - name: Format - run: golangci-lint fmt --diff diff --git a/CHANGELOG.md b/CHANGELOG.md index cb87d50..65da421 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,10 +7,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] -### Fixed - -* `config list -j` works again. `-j` was a shorthand for `--json` on `config list` before 4.7.0 promoted `--json` to a global flag, and removing the global shorthand in 4.8.2 took `config list -j` with it. The shorthand is registered on `config list` again; `--json` continues to work everywhere. - ## [4.8.2] - 2026-08-10 ### Fixed diff --git a/app/app.go b/app/app.go index 53d4ad7..5b8c263 100644 --- a/app/app.go +++ b/app/app.go @@ -1075,7 +1075,7 @@ func (a *App) GetECSEvents(service string) ([]ecstypes.ServiceEvent, error) { // 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` / `--external-database`. +// `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 { @@ -1094,8 +1094,7 @@ func noDatabaseConfiguredErrorFromConfig(appName string, configVars ConfigVariab 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` (or recreate the app with `--external-database postgres|mysql`) "+ - "to enable database commands", + "run `apppack modify app %s` to enable database commands", appName, appName, ) } diff --git a/app/db_test.go b/app/db_test.go index a9dbf7a..a5a8764 100644 --- a/app/db_test.go +++ b/app/db_test.go @@ -12,19 +12,24 @@ func TestNoDatabaseConfiguredErrorFromConfig(t *testing.T) { t.Parallel() tests := []struct { - name string - configVars ConfigVariables - wantErrSub string + name string + configVars ConfigVariables + wantErrSub string + notWantErrSub string }{ { - name: "DATABASE_URL present -- points at modify app / --external-database", + name: "DATABASE_URL present -- points at modify app", configVars: ConfigVariables{{Name: "DATABASE_URL", Value: "postgres://example"}}, wantErrSub: "apppack modify app myapp", }, { - name: "DATABASE_URL present -- mentions --external-database", - configVars: ConfigVariables{{Name: "DATABASE_URL", Value: "postgres://example"}}, - wantErrSub: "--external-database", + // 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", @@ -50,6 +55,10 @@ func TestNoDatabaseConfiguredErrorFromConfig(t *testing.T) { 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/cmd/config.go b/cmd/config.go index 9724917..da62fc6 100644 --- a/cmd/config.go +++ b/cmd/config.go @@ -242,12 +242,6 @@ func init() { configCmd.AddCommand(setCmd) configCmd.AddCommand(unsetCmd) configCmd.AddCommand(configListCmd) - // `config list` carried its own --json/-j long before --json was promoted to a - // root persistent flag, so re-register it locally to keep `-j` working here. - // The shorthand can't live on the root flag: `db load` already owns -j for - // --jobs, and the two collide the moment cobra merges the flag sets. - // Same variable as the root flag, so `--json` behaves identically either way. - configListCmd.Flags().BoolVarP(&AsJSON, "json", "j", false, "output as JSON") configCmd.AddCommand(configExportCmd) configExportCmd.Flags().BoolVar(&includeManagedVars, "all", diff --git a/cmd/flags_test.go b/cmd/flags_test.go deleted file mode 100644 index 0b2e27d..0000000 --- a/cmd/flags_test.go +++ /dev/null @@ -1,73 +0,0 @@ -package cmd - -import ( - "fmt" - "testing" - - "github.com/spf13/cobra" - "github.com/spf13/pflag" -) - -// walkCommands visits c and every command beneath it. -func walkCommands(c *cobra.Command, fn func(*cobra.Command)) { - fn(c) - for _, sub := range c.Commands() { - walkCommands(sub, fn) - } -} - -// TestNoFlagShorthandCollisions guards against a persistent flag on a parent -// command claiming a shorthand that a subcommand's local flag already uses. -// -// Cobra registers local and persistent flags in separate FlagSets, so such a -// collision is invisible at init time. It only surfaces when cobra merges the -// two sets, which happens the first time the subcommand actually runs -- at -// which point pflag panics and the command is unusable. That is how a global -// `--json`/`-j` broke every `apppack db load` invocation for six weeks -// (4.7.0 through 4.8.1). -// -// Calling Flags(), LocalFlags() and InheritedFlags() on each command forces -// that merge, so the collision fails here instead of in a user's terminal. -func TestNoFlagShorthandCollisions(t *testing.T) { - var collisions []string - - walkCommands(rootCmd, func(c *cobra.Command) { - defer func() { - if r := recover(); r != nil { - collisions = append(collisions, fmt.Sprintf("%s: %v", c.CommandPath(), r)) - } - }() - - noop := func(*pflag.Flag) {} - c.Flags().VisitAll(noop) - c.LocalFlags().VisitAll(noop) - c.InheritedFlags().VisitAll(noop) - }) - - for _, c := range collisions { - t.Errorf("flag shorthand collision: %s", c) - } -} - -// TestShorthandsUniquePerCommand asserts the same invariant declaratively, so -// it still holds if a future pflag release downgrades the collision from a -// panic to silent shadowing. -func TestShorthandsUniquePerCommand(t *testing.T) { - walkCommands(rootCmd, func(c *cobra.Command) { - defer func() { _ = recover() }() // collisions are reported by the test above - - byShorthand := map[string]string{} - c.Flags().VisitAll(func(f *pflag.Flag) { - if f.Shorthand == "" { - return - } - if existing, taken := byShorthand[f.Shorthand]; taken && existing != f.Name { - t.Errorf( - "%s: shorthand -%s is claimed by both --%s and --%s", - c.CommandPath(), f.Shorthand, existing, f.Name, - ) - } - byShorthand[f.Shorthand] = f.Name - }) - }) -} diff --git a/cmd/json_test.go b/cmd/json_test.go index 091d638..3a4301a 100644 --- a/cmd/json_test.go +++ b/cmd/json_test.go @@ -263,23 +263,6 @@ func TestRootJSONPersistentFlag(t *testing.T) { } } -// TestConfigListJSONShorthand verifies `config list -j` still works. The -// shorthand lived on this command before --json was promoted to a root -// persistent flag, so it has to stay registered locally -- it can't move to -// the root flag, which would collide with `db load --jobs`. -func TestConfigListJSONShorthand(t *testing.T) { - t.Parallel() - - flag := configListCmd.Flags().Lookup("json") - if flag == nil { - t.Fatal("expected --json flag on configListCmd, not found") - } - - if flag.Shorthand != "j" { - t.Errorf("expected -j shorthand on `config list --json`, got %q", flag.Shorthand) - } -} - // captureStdout redirects os.Stdout to a pipe, runs fn, then restores stdout // and returns what was written. func captureStdout(t *testing.T, fn func()) string { From d2b81c41095388c15d5ddccd86d05e783d4d834f Mon Sep 17 00:00:00 2001 From: Brendan Smith Date: Mon, 24 Aug 2026 12:17:52 -0400 Subject: [PATCH 4/8] Restore files dropped by a mis-staged commit The previous commit was made against an index that already held staged deletions from a concurrent rebase in this worktree, so it removed the go vet/fmt CI gate, the flag-shorthand collision tests, and unrelated CHANGELOG and config entries. Restore all five from main; none of them have anything to do with this branch. --- .github/workflows/go_tests.yml | 21 ++++++++++ CHANGELOG.md | 4 ++ cmd/config.go | 6 +++ cmd/flags_test.go | 73 ++++++++++++++++++++++++++++++++++ cmd/json_test.go | 17 ++++++++ 5 files changed, 121 insertions(+) create mode 100644 cmd/flags_test.go diff --git a/.github/workflows/go_tests.yml b/.github/workflows/go_tests.yml index a55b9d4..38389b6 100644 --- a/.github/workflows/go_tests.yml +++ b/.github/workflows/go_tests.yml @@ -16,3 +16,24 @@ jobs: go-version: "1.25" - name: Test run: go test ./... -v + + checks: + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@v3 + - name: Set up Go + uses: actions/setup-go@v4 + with: + go-version: "1.25" + - name: Vet + run: go vet ./... + # Same tool and subcommand as `make fmt`, so a clean local run is a + # clean CI run. golangci-lint-action can't be used here: it only ever + # invokes `golangci-lint run`, with no way to select `fmt`. + # Deliberately not `golangci-lint run` -- that still reports ~29 + # pre-existing errcheck findings and would land red. + - name: Install golangci-lint + run: go install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.11.3 + - name: Format + run: golangci-lint fmt --diff diff --git a/CHANGELOG.md b/CHANGELOG.md index 65da421..cb87d50 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +* `config list -j` works again. `-j` was a shorthand for `--json` on `config list` before 4.7.0 promoted `--json` to a global flag, and removing the global shorthand in 4.8.2 took `config list -j` with it. The shorthand is registered on `config list` again; `--json` continues to work everywhere. + ## [4.8.2] - 2026-08-10 ### Fixed diff --git a/cmd/config.go b/cmd/config.go index da62fc6..9724917 100644 --- a/cmd/config.go +++ b/cmd/config.go @@ -242,6 +242,12 @@ func init() { configCmd.AddCommand(setCmd) configCmd.AddCommand(unsetCmd) configCmd.AddCommand(configListCmd) + // `config list` carried its own --json/-j long before --json was promoted to a + // root persistent flag, so re-register it locally to keep `-j` working here. + // The shorthand can't live on the root flag: `db load` already owns -j for + // --jobs, and the two collide the moment cobra merges the flag sets. + // Same variable as the root flag, so `--json` behaves identically either way. + configListCmd.Flags().BoolVarP(&AsJSON, "json", "j", false, "output as JSON") configCmd.AddCommand(configExportCmd) configExportCmd.Flags().BoolVar(&includeManagedVars, "all", diff --git a/cmd/flags_test.go b/cmd/flags_test.go new file mode 100644 index 0000000..0b2e27d --- /dev/null +++ b/cmd/flags_test.go @@ -0,0 +1,73 @@ +package cmd + +import ( + "fmt" + "testing" + + "github.com/spf13/cobra" + "github.com/spf13/pflag" +) + +// walkCommands visits c and every command beneath it. +func walkCommands(c *cobra.Command, fn func(*cobra.Command)) { + fn(c) + for _, sub := range c.Commands() { + walkCommands(sub, fn) + } +} + +// TestNoFlagShorthandCollisions guards against a persistent flag on a parent +// command claiming a shorthand that a subcommand's local flag already uses. +// +// Cobra registers local and persistent flags in separate FlagSets, so such a +// collision is invisible at init time. It only surfaces when cobra merges the +// two sets, which happens the first time the subcommand actually runs -- at +// which point pflag panics and the command is unusable. That is how a global +// `--json`/`-j` broke every `apppack db load` invocation for six weeks +// (4.7.0 through 4.8.1). +// +// Calling Flags(), LocalFlags() and InheritedFlags() on each command forces +// that merge, so the collision fails here instead of in a user's terminal. +func TestNoFlagShorthandCollisions(t *testing.T) { + var collisions []string + + walkCommands(rootCmd, func(c *cobra.Command) { + defer func() { + if r := recover(); r != nil { + collisions = append(collisions, fmt.Sprintf("%s: %v", c.CommandPath(), r)) + } + }() + + noop := func(*pflag.Flag) {} + c.Flags().VisitAll(noop) + c.LocalFlags().VisitAll(noop) + c.InheritedFlags().VisitAll(noop) + }) + + for _, c := range collisions { + t.Errorf("flag shorthand collision: %s", c) + } +} + +// TestShorthandsUniquePerCommand asserts the same invariant declaratively, so +// it still holds if a future pflag release downgrades the collision from a +// panic to silent shadowing. +func TestShorthandsUniquePerCommand(t *testing.T) { + walkCommands(rootCmd, func(c *cobra.Command) { + defer func() { _ = recover() }() // collisions are reported by the test above + + byShorthand := map[string]string{} + c.Flags().VisitAll(func(f *pflag.Flag) { + if f.Shorthand == "" { + return + } + if existing, taken := byShorthand[f.Shorthand]; taken && existing != f.Name { + t.Errorf( + "%s: shorthand -%s is claimed by both --%s and --%s", + c.CommandPath(), f.Shorthand, existing, f.Name, + ) + } + byShorthand[f.Shorthand] = f.Name + }) + }) +} diff --git a/cmd/json_test.go b/cmd/json_test.go index 3a4301a..091d638 100644 --- a/cmd/json_test.go +++ b/cmd/json_test.go @@ -263,6 +263,23 @@ func TestRootJSONPersistentFlag(t *testing.T) { } } +// TestConfigListJSONShorthand verifies `config list -j` still works. The +// shorthand lived on this command before --json was promoted to a root +// persistent flag, so it has to stay registered locally -- it can't move to +// the root flag, which would collide with `db load --jobs`. +func TestConfigListJSONShorthand(t *testing.T) { + t.Parallel() + + flag := configListCmd.Flags().Lookup("json") + if flag == nil { + t.Fatal("expected --json flag on configListCmd, not found") + } + + if flag.Shorthand != "j" { + t.Errorf("expected -j shorthand on `config list --json`, got %q", flag.Shorthand) + } +} + // captureStdout redirects os.Stdout to a pipe, runs fn, then restores stdout // and returns what was written. func captureStdout(t *testing.T, fn func()) string { From a1109072d100b92425f374b3b084b321c0696a2a Mon Sep 17 00:00:00 2001 From: Brendan Smith Date: Wed, 26 Aug 2026 09:44:30 -0400 Subject: [PATCH 5/8] Point users at `modify app` after DATABASE_URL is set Setting DATABASE_URL is the obvious move when pointing an app at an externally-managed database, but on its own it does nothing for `apppack db shell`/`db dump`/`db load` -- those need the db-utils resources, which only appear when the stack is updated. Nothing told the user that second step existed, so the discoverable path dead-ended at a config variable that looked sufficient. Print a follow-up hint from `config set` and `config import` when DATABASE_URL lands on an app whose db utils aren't enabled yet. Stays quiet when an engine is already set (managed database, or a previous `modify app`), and for pipelines/review apps, where external databases are gated off in CloudFormation anyway. Advisory only: the variable is already stored by the time we're called, so a settings-load failure just skips the hint rather than failing a command that succeeded. Suggested by ipmb in review of #173. --- cmd/config.go | 65 +++++++++++++++++++++++++++++++ cmd/config_test.go | 96 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 161 insertions(+) create mode 100644 cmd/config_test.go 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..ea41387 --- /dev/null +++ b/cmd/config_test.go @@ -0,0 +1,96 @@ +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 { + 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") + } +} From aed96347fd6433712d9b00786dd70879f40d526d Mon Sep 17 00:00:00 2001 From: Brendan Smith Date: Tue, 1 Sep 2026 10:31:47 -0400 Subject: [PATCH 6/8] Fix DeepSource findings on PR #173 - Add `tt := tt` loop-variable copies in app/db_test.go and cmd/config_test.go table tests. These are false-positive VET-V0010 hits: go.mod requires go 1.25.4, where each `for` iteration already gets its own variable, but DeepSource's loopclosure analyzer still assumes pre-1.22 semantics whenever it sees t.Parallel() inside a t.Run closure. Matches the existing skipcq-free remediation used elsewhere in the repo. - Suppress CRT-P0003 on detectExternalDatabaseEngine and the fetchDatabaseURL function variable in stacks/app_pipeline.go via a trailing `// skipcq: CRT-P0003` comment, following the precedent in app/utils.go. Passing aws.Config by value is the SDK convention used throughout this codebase, so the signature stays unchanged. - Drop the bogus t.Helper() calls from TestEngineFromDatabaseURL, TestDetectExternalDatabaseEngine, and TestDetectExternalDatabaseEngineIdempotent -- t.Helper() has no effect on a top-level Test function and was left over from a copy of the table-test template. TestEngineFromDatabaseURL is a pure function test, so it gets t.Parallel() like its siblings. The other two stub the package-level fetchDatabaseURL variable and must not run in parallel with each other, so they stay serial with no t.Parallel() call (their subtests were never parallel either). --- app/db_test.go | 1 + cmd/config_test.go | 1 + stacks/app_pipeline.go | 4 ++-- stacks/app_pipeline_test.go | 6 +----- 4 files changed, 5 insertions(+), 7 deletions(-) diff --git a/app/db_test.go b/app/db_test.go index a5a8764..97b5a50 100644 --- a/app/db_test.go +++ b/app/db_test.go @@ -44,6 +44,7 @@ func TestNoDatabaseConfiguredErrorFromConfig(t *testing.T) { } for _, tt := range tests { + tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() diff --git a/cmd/config_test.go b/cmd/config_test.go index ea41387..0761666 100644 --- a/cmd/config_test.go +++ b/cmd/config_test.go @@ -68,6 +68,7 @@ func TestShouldHintEnableDBUtils(t *testing.T) { } for _, tt := range tests { + tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() diff --git a/stacks/app_pipeline.go b/stacks/app_pipeline.go index 6adb209..eaf6549 100644 --- a/stacks/app_pipeline.go +++ b/stacks/app_pipeline.go @@ -147,7 +147,7 @@ func externalDatabaseConfigPath(appName string) string { // 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) { +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 @@ -199,7 +199,7 @@ func engineFromDatabaseURL(rawURL string) (engine, unrecognizedScheme string) { // (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) { +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 != "" { diff --git a/stacks/app_pipeline_test.go b/stacks/app_pipeline_test.go index f15d770..fe843fe 100644 --- a/stacks/app_pipeline_test.go +++ b/stacks/app_pipeline_test.go @@ -679,7 +679,7 @@ func TestSelectRedisStack(t *testing.T) { // --- engineFromDatabaseURL --- func TestEngineFromDatabaseURL(t *testing.T) { - t.Helper() + t.Parallel() tests := []struct { name string @@ -733,8 +733,6 @@ func TestEngineFromDatabaseURL(t *testing.T) { // SetInternalFields. The SSM read is stubbed via the fetchDatabaseURL function // variable so these run with no AWS calls. func TestDetectExternalDatabaseEngine(t *testing.T) { - t.Helper() - tests := []struct { name string params AppStackParameters @@ -824,8 +822,6 @@ func TestDetectExternalDatabaseEngine(t *testing.T) { // a managed database stays "" forever, and an app whose DATABASE_URL is later unset goes // back to "". func TestDetectExternalDatabaseEngineIdempotent(t *testing.T) { - t.Helper() - originalFetch := fetchDatabaseURL defer func() { fetchDatabaseURL = originalFetch }() From 6a65172f7c3efda5286d5cb8112f86fc6ca5c014 Mon Sep 17 00:00:00 2001 From: Brendan Smith Date: Tue, 1 Sep 2026 14:20:40 -0400 Subject: [PATCH 7/8] Parallelize TestEngineFromDatabaseURL subtests Adding t.Parallel() to the parent test in aed9634 tripped GO-W6007: DeepSource requires subtests to call t.Parallel() too once the parent does. TestEngineFromDatabaseURL's subtests only exercise the pure engineFromDatabaseURL function, so this is safe. Add the standard tt := tt loop-variable copy alongside it so the added closure over tt/t.Parallel() doesn't re-trigger the VET-V0010 loopclosure false positive on this file. TestDetectExternalDatabaseEngine and TestDetectExternalDatabaseEngineIdempotent stay serial since they mutate the package-level fetchDatabaseURL stub. --- stacks/app_pipeline_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/stacks/app_pipeline_test.go b/stacks/app_pipeline_test.go index fe843fe..8e5619e 100644 --- a/stacks/app_pipeline_test.go +++ b/stacks/app_pipeline_test.go @@ -713,7 +713,10 @@ func TestEngineFromDatabaseURL(t *testing.T) { } 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 { From b3e9afe4977eb9e2ed054b037c87bd01bcb2fcf5 Mon Sep 17 00:00:00 2001 From: Brendan Smith Date: Wed, 2 Sep 2026 12:39:30 -0400 Subject: [PATCH 8/8] Fix db shell for externally-managed MySQL databases DBShellTaskInfo() built `mysql --database=`, which only works for a managed AppPack database (named after the app). An externally- managed MySQL (e.g. PlanetScale) has whatever database name is in its DATABASE_URL, so the connection failed. For non-review apps, use a bare `mysql` and let the db-utils image supply the database, the same way psql already does via ~/.pg_service.conf. Review-app behavior is untouched: review apps cannot use external databases (the CloudFormation condition requires IsApp), so they keep the explicit `--database=-pr` form byte-for-byte. db dump and db load are unaffected -- dump-to-s3.sh/load-from-s3.sh already derive the database name from DATABASE_URL via $NAME. Deploy ordering: this changes the managed-database command too (bare `mysql` instead of `mysql --database=`), which only works once the db-utils image writes `database=$NAME` into ~/.my.cnf (in flight as apppackio/apppack-db-utils#5). This is safe because the image is served from a mutable tag (public.ecr.aws/d9q4v8a4/apppack-db-utils:mysql, rebuilt from main) with no version pinning, and db-utils ships ahead of the CLI -- so there's no window where the CLI's new bare `mysql` command reaches a db-utils image that doesn't yet resolve the database from DATABASE_URL. --- app/app.go | 11 ++++-- app/shelltask_test.go | 86 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+), 4 deletions(-) diff --git a/app/app.go b/app/app.go index 5b8c263..6b6b862 100644 --- a/app/app.go +++ b/app/app.go @@ -1193,12 +1193,15 @@ 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 == "" { 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) + } + }) + } +}