diff --git a/TEMPLATES.md b/TEMPLATES.md index 8ae908346..18f2c3dfa 100644 --- a/TEMPLATES.md +++ b/TEMPLATES.md @@ -49,6 +49,55 @@ ALTER TABLE `products` ADD INDEX `idx_category_price`(`category`, `price`); πŸ“‹ **Plan**: **2** tables to create, **1** table to alter +--- + +▢️ **To apply** all schema changes from this PR, comment: +``` +schemabot apply -e staging +``` + + + +
+MySQL Plan (Ignored Namespaces) + + +## Schema Change Plan β€” Staging + +**Database**: `testapp` | **Type**: `MySQL` | **Schema Name**: `testapp` + +*Requested by @jackjackbits at 2026-01-01 00:00:00 UTC Β· planned from [`abcdef1`](https://github.com/block/schemabot/commit/abcdef1234567890abcdef1234567890abcdef12)* + +```sql +CREATE TABLE `users` ( + `id` bigint unsigned NOT NULL AUTO_INCREMENT, + `email` varchar(255) NOT NULL, + `created_at` timestamp DEFAULT current_timestamp(), + PRIMARY KEY(`id`), + INDEX `idx_email`(`email`) +) ENGINE InnoDB, + CHARSET utf8mb4, + COLLATE utf8mb4_0900_ai_ci; + +CREATE TABLE `orders` ( + `id` bigint unsigned NOT NULL AUTO_INCREMENT, + `user_id` bigint NOT NULL, + `total_cents` bigint NOT NULL, + `status` varchar(50) NOT NULL DEFAULT 'pending', + PRIMARY KEY(`id`), + INDEX `idx_user_id`(`user_id`) +) ENGINE InnoDB, + CHARSET utf8mb4, + COLLATE utf8mb4_0900_ai_ci; + +ALTER TABLE `products` ADD INDEX `idx_category_price`(`category`, `price`); +``` + +πŸ“‹ **Plan**: **2** tables to create, **1** table to alter + +ℹ️ Namespaces excluded from this plan by `ignore_namespaces`: `local_fixtures` + + --- ▢️ **To apply** all schema changes from this PR, comment: diff --git a/docs/github-app-setup.md b/docs/github-app-setup.md index b2c6a2a1f..1125960f8 100644 --- a/docs/github-app-setup.md +++ b/docs/github-app-setup.md @@ -182,6 +182,7 @@ type: mysql |-------|----------|-------------| | `database` | Yes | Must match a database name in your SchemaBot server config | | `type` | Yes | `"mysql"` or `"vitess"` | +| `ignore_namespaces` | No | Namespace subdirectories to exclude from plans, applies, and checks (see [Ignoring Namespaces](namespaces.md#ignoring-namespaces)) | Environment availability and promotion order are configured on the SchemaBot server. diff --git a/docs/namespaces.md b/docs/namespaces.md index db41ed23a..f94ee1d8f 100644 --- a/docs/namespaces.md +++ b/docs/namespaces.md @@ -14,8 +14,12 @@ - [`$ENV` Substitution in Namespace Names](#env-substitution-in-namespace-names) - [Example](#example) - [Rules](#rules) -- [Per-Target Schema Overrides](#per-target-schema-overrides) +- [Ignoring Namespaces](#ignoring-namespaces) - [Rules](#rules-1) + - [Exclusions are disclosed](#exclusions-are-disclosed) + - [MySQL target DSN requirements](#mysql-target-dsn-requirements) +- [Per-Target Schema Overrides](#per-target-schema-overrides) + - [Rules](#rules-2) - [Summary](#summary) - [How Namespaces Flow Through the System](#how-namespaces-flow-through-the-system) @@ -220,6 +224,61 @@ schemabot plan -s myapp/schema -e production - You can mix `$ENV` directories with regular directories in the subdirectory layout. - When creating the directory from a shell, quote the name to prevent shell expansion: `mkdir 'bikeshare_$ENV'` +## Ignoring Namespaces + +Some schema roots contain a namespace directory that should never be reconciled against a live database. A common example is a Vitess keyspace that exists only in local test infrastructure: the repository carries its schema files so local tooling can create the keyspace, but no real environment has it (or the environment keeps it intentionally empty). Without an exclusion, every plan would propose creating those tables. + +List such namespaces under `ignore_namespaces` in `schemabot.yaml`: + +``` +myapp/schema/ +β”œβ”€β”€ schemabot.yaml +β”œβ”€β”€ commerce/ +β”‚ β”œβ”€β”€ orders.sql +β”‚ └── vschema.json +└── commerce_test/ ← ignored: never planned or applied + β”œβ”€β”€ fixtures.sql + └── vschema.json +``` + +```yaml +# schemabot.yaml +database: commerce +type: vitess +ignore_namespaces: + - commerce_test +``` + +### Rules + +- Entries are bare namespace names, not paths. An entry containing `/` or `\` (e.g., `schema/commerce_test`) is rejected when the config is loaded. +- Ignored namespaces are excluded from plans, applies, and merge-gate checks. This applies to both the GitHub PR flow and the CLI (`schemabot plan` / `schemabot apply` read the same `schemabot.yaml`). +- `$ENV` substitution applies to entries the same way it applies to directory names: `fixtures_$ENV` ignores the `fixtures_staging` namespace when planning for staging. +- Matching is exact and case-sensitive. An entry that matches no namespace directory excludes nothing; the plan proceeds and the unmatched entry is reported (a CLI warning, a server-side log) so a typo or stale entry is visible. +- Ignoring every namespace in the schema root is an error: the plan fails rather than reconciling an empty desired state. +- Ignoring a namespace does not exempt the directory from layout validation; a schema root mixing flat files and subdirectories is still rejected. + +### Exclusions are disclosed + +Every plan that excluded namespaces says so: the PR plan comment renders an +`ℹ️ Namespaces excluded from this plan by ignore_namespaces: …` line under the +plan summary (also on "no changes" results, so a withheld namespace is +distinguishable from an unchanged one), and the CLI prints the same disclosure +for `plan` and `apply`. +When reviewing a PR that *introduces* an `ignore_namespaces` entry, the +disclosure plus the config diff is the review surface: the plan stops +reconciling that namespace from this PR onward. + +### MySQL target DSN requirements + +For MySQL targets, `ignore_namespaces` requires a **namespace-free** target +DSN (one that does not name a database), where each namespace directory is +diffed against its own database. A DSN that already names a database diffs +the whole database as one unit: an ignored namespace's live tables would +have no declaring files and the diff would plan them as `DROP TABLE`, the +inverse of "ignore". SchemaBot refuses this combination, and the plan fails with +an error asking for a namespace-free DSN or removal of `ignore_namespaces`. + ## Per-Target Schema Overrides `$ENV` substitution handles physical schema names that vary by *environment*. When names vary by *deployment within one environment* β€” several regional clusters in the same environment naming the schema `bikeshare_qa`, `bikeshare_eu_qa`, and `bikeshare_us_qa` β€” one schema directory cannot express the variance, and copying the directory per region would triple the source of truth. @@ -256,6 +315,7 @@ The canonical namespace stays the name everywhere SchemaBot stores or shows it | MySQL, different databases | 1 per database | 1 each | separate directories | | Vitess, multiple keyspaces | 1 | many | `commerce/`, `commerce_sharded/` | | Environment-specific namespace | 1 | 1 per env | `bikeshare_$ENV/` | +| Repo-only namespace (never deployed) | 1 | all except ignored | `ignore_namespaces: [commerce_test]` | | Deployment-specific physical schema | 1 | 1 canonical | `bikeshare/` + per-target `schema_overrides` | ## How Namespaces Flow Through the System diff --git a/e2e/local/vitess_test.go b/e2e/local/vitess_test.go index 51cb571b6..49846c177 100644 --- a/e2e/local/vitess_test.go +++ b/e2e/local/vitess_test.go @@ -1500,7 +1500,7 @@ func TestVitess_Apply_DeferDeploy(t *testing.T) { schemaDir := newVitessSchemaDir(t, vitessSchemaWithOverrides(map[string]string{ "testapp_sharded/users.sql": usersSchemaWithColumn(colName), })) - planResp, err := client.CallPlanAPI(endpoint, vitessDB, "vitess", "staging", schemaDir, "", 0) + planResp, _, err := client.CallPlanAPI(endpoint, vitessDB, "vitess", "staging", schemaDir, "", 0, nil) require.NoError(t, err) require.NotEmpty(t, planResp.PlanID) require.NotEmpty(t, planResp.Changes, "expected plan to have changes") @@ -1554,7 +1554,7 @@ func TestVitess_Apply_DeferDeploy_StartTooEarly(t *testing.T) { schemaDir := newVitessSchemaDir(t, vitessSchemaWithOverrides(map[string]string{ "testapp_sharded/users.sql": usersSchemaWithColumn(colName), })) - planResp, err := client.CallPlanAPI(endpoint, vitessDB, "vitess", "staging", schemaDir, "", 0) + planResp, _, err := client.CallPlanAPI(endpoint, vitessDB, "vitess", "staging", schemaDir, "", 0, nil) require.NoError(t, err) require.NotEmpty(t, planResp.PlanID) diff --git a/e2e/testutil/apply.go b/e2e/testutil/apply.go index ae7a56229..1775bcdc0 100644 --- a/e2e/testutil/apply.go +++ b/e2e/testutil/apply.go @@ -20,7 +20,7 @@ import ( // subdirectory as the namespace. func groupFiles(t *testing.T, files map[string]string, database string) map[string]*apitypes.SchemaFiles { t.Helper() - grouped, err := schema.GroupFilesByNamespace(files, database, "development") + grouped, _, err := schema.GroupFilesByNamespace(files, database, "development", nil) require.NoError(t, err, "group schema files by namespace") result := make(map[string]*apitypes.SchemaFiles, len(grouped)) for ns, nsFiles := range grouped { diff --git a/pkg/api/plan_handlers.go b/pkg/api/plan_handlers.go index d33adaab7..510d0d9c1 100644 --- a/pkg/api/plan_handlers.go +++ b/pkg/api/plan_handlers.go @@ -47,6 +47,11 @@ type PlanRequest struct { // Optional β€” absent for non-webhook callers (e.g. CLI plan invocations without a PR). HeadSHA *string `json:"head_sha,omitempty"` SchemaPath string `json:"-"` + // IgnoredNamespaces lists the namespaces the caller removed from + // SchemaFiles per the config's ignore_namespaces, resolved for the + // environment. Forwarded to the data plane so it can refuse engine shapes + // that cannot honor the exclusion. + IgnoredNamespaces []string `json:"ignored_namespaces,omitempty"` // SourceTrusted is set by the GitHub webhook path after SchemaBot has // discovered the PR source itself. It is deliberately not JSON-decodable: @@ -546,13 +551,14 @@ func (s *Service) ExecutePlanProto(ctx context.Context, req PlanRequest) (*ternv } ternReq := &ternv1.PlanRequest{ - Database: req.Database, - Type: resolvedTarget.DatabaseType, - SchemaFiles: req.SchemaFiles, - Repository: req.Repository, - Environment: req.Environment, - Target: resolvedTarget.Target, - SchemaPath: trustedSchemaPath, + Database: req.Database, + Type: resolvedTarget.DatabaseType, + SchemaFiles: req.SchemaFiles, + Repository: req.Repository, + Environment: req.Environment, + Target: resolvedTarget.Target, + SchemaPath: trustedSchemaPath, + IgnoredNamespaces: req.IgnoredNamespaces, } if req.PullRequest != nil { ternReq.PullRequest = *req.PullRequest @@ -569,6 +575,19 @@ func (s *Service) ExecutePlanProto(ctx context.Context, req PlanRequest) (*ternv "is_remote", client.IsRemote(), "schema_file_count", len(req.SchemaFiles), ) + if len(req.IgnoredNamespaces) > 0 { + // The desired state is deliberately partial: the caller withheld these + // namespaces per the config's ignore_namespaces. Recorded so an operator + // tracing "why does this plan not touch namespace X" finds the answer in + // server logs. + s.logger.Info("plan request excludes ignored namespaces", + "database", req.Database, + "environment", req.Environment, + "deployment", deployment, + "repository", req.Repository, + "ignored_namespaces", req.IgnoredNamespaces, + ) + } resp, err := client.Plan(ctx, ternReq) if err != nil { diff --git a/pkg/apitypes/apitypes.go b/pkg/apitypes/apitypes.go index 3cf5ff5fe..4da25b0b4 100644 --- a/pkg/apitypes/apitypes.go +++ b/pkg/apitypes/apitypes.go @@ -398,6 +398,12 @@ type PlanRequest struct { // cross-delivery race where HEAD advances between plan and confirm. // Optional β€” absent for non-webhook callers (e.g. CLI plan invocations without a PR). HeadSHA *string `json:"head_sha,omitempty"` + // IgnoredNamespaces lists the namespaces the caller removed from + // SchemaFiles per the config's ignore_namespaces, resolved for the + // environment. The data plane refuses engine shapes that diff the whole + // target as one unit (a database-scoped MySQL DSN), where a withheld + // namespace's live tables would otherwise be planned as drops. + IgnoredNamespaces []string `json:"ignored_namespaces,omitempty"` } // ApplyRequest is the HTTP request body for POST /api/apply. diff --git a/pkg/cmd/client/client.go b/pkg/cmd/client/client.go index e0caee4e6..0bea5488a 100644 --- a/pkg/cmd/client/client.go +++ b/pkg/cmd/client/client.go @@ -143,26 +143,44 @@ func CallPullSchemaAPIWithOptions(endpoint, database, dbType, environment string // CallPlanAPI calls the plan API by reading .sql files from schemaDir. // Files are grouped by namespace: subdirectories become namespace keys, -// flat files use the directory name as the namespace. -func CallPlanAPI(endpoint, database, dbType, environment, schemaDir, repo string, pr int) (*apitypes.PlanResponse, error) { - schemaFiles, err := ReadSchemaFiles(schemaDir, environment) +// flat files use the directory name as the namespace. Namespaces listed in +// ignoreNamespaces are excluded from the plan request. The second return +// value lists the namespaces actually removed by ignoreNamespaces so callers +// can disclose the exclusion alongside the plan. +func CallPlanAPI(endpoint, database, dbType, environment, schemaDir, repo string, pr int, ignoreNamespaces []string) (*apitypes.PlanResponse, []string, error) { + schemaFiles, ignored, err := ReadSchemaFiles(schemaDir, environment, ignoreNamespaces) if err != nil { - return nil, fmt.Errorf("read schema files: %w", err) + return nil, nil, fmt.Errorf("read schema files: %w", err) } if len(schemaFiles) == 0 { - return nil, fmt.Errorf("no .sql files found in %s", schemaDir) + if len(ignored) > 0 { + return nil, ignored, fmt.Errorf("no .sql files found in %s after excluding ignored namespaces %v", schemaDir, ignored) + } + return nil, nil, fmt.Errorf("no .sql files found in %s", schemaDir) + } + resp, err := postPlanRequest(endpoint, database, dbType, environment, schemaFiles, repo, pr, ignored) + if err != nil { + return nil, ignored, err } - return CallPlanAPIWithFiles(endpoint, database, dbType, environment, schemaFiles, repo, pr) + return resp, ignored, nil } // CallPlanAPIWithFiles calls the plan API with pre-loaded, namespace-grouped schema files. func CallPlanAPIWithFiles(endpoint, database, dbType, environment string, schemaFiles map[string]*apitypes.SchemaFiles, repo string, pr int) (*apitypes.PlanResponse, error) { + return postPlanRequest(endpoint, database, dbType, environment, schemaFiles, repo, pr, nil) +} + +// postPlanRequest posts a plan request. ignoredNamespaces names the +// namespaces removed from schemaFiles before the call β€” the server needs +// them to refuse engine shapes that cannot honor the exclusion. +func postPlanRequest(endpoint, database, dbType, environment string, schemaFiles map[string]*apitypes.SchemaFiles, repo string, pr int, ignoredNamespaces []string) (*apitypes.PlanResponse, error) { req := apitypes.PlanRequest{ - Database: database, - Type: dbType, - Environment: environment, - SchemaFiles: schemaFiles, - Repository: repo, + Database: database, + Type: dbType, + Environment: environment, + SchemaFiles: schemaFiles, + Repository: repo, + IgnoredNamespaces: ignoredNamespaces, } if pr != 0 { prVal := int32(pr) @@ -291,13 +309,17 @@ func CheckActiveSchemaChange(endpoint, database, environment string) (*ActiveSch // The environment parameter enables $ENV substitution in namespace names. // If non-empty, any "$ENV" in directory names or the default namespace is // replaced with the environment value (e.g., "bikeshare_$ENV" β†’ "bikeshare_staging"). -func ReadSchemaFiles(dir string, environment string) (map[string]*apitypes.SchemaFiles, error) { +// +// Namespaces listed in ignoreNamespaces (schemabot.yaml ignore_namespaces) are +// excluded from the result. The second return value lists the namespace keys +// actually removed by ignoreNamespaces, sorted. +func ReadSchemaFiles(dir string, environment string, ignoreNamespaces []string) (map[string]*apitypes.SchemaFiles, []string, error) { // Collect all files as relativePath β†’ content rawFiles := make(map[string]string) entries, err := os.ReadDir(dir) if err != nil { - return nil, err + return nil, nil, err } for _, entry := range entries { @@ -313,7 +335,7 @@ func ReadSchemaFiles(dir string, environment string) (map[string]*apitypes.Schem // Read schema files inside the subdirectory subEntries, err := os.ReadDir(filepath.Join(dir, entry.Name())) if err != nil { - return nil, fmt.Errorf("read subdirectory %s: %w", entry.Name(), err) + return nil, nil, fmt.Errorf("read subdirectory %s: %w", entry.Name(), err) } for _, sub := range subEntries { if sub.IsDir() { @@ -327,7 +349,7 @@ func ReadSchemaFiles(dir string, environment string) (map[string]*apitypes.Schem relPath := path.Join(entry.Name(), sub.Name()) content, err := os.ReadFile(filepath.Join(dir, entry.Name(), sub.Name())) if err != nil { - return nil, fmt.Errorf("read %s: %w", relPath, err) + return nil, nil, fmt.Errorf("read %s: %w", relPath, err) } rawFiles[relPath] = string(content) } @@ -338,16 +360,16 @@ func ReadSchemaFiles(dir string, environment string) (map[string]*apitypes.Schem } content, err := os.ReadFile(filepath.Join(dir, entry.Name())) if err != nil { - return nil, fmt.Errorf("read %s: %w", entry.Name(), err) + return nil, nil, fmt.Errorf("read %s: %w", entry.Name(), err) } rawFiles[entry.Name()] = string(content) } // Group by namespace using the shared helper. // For flat files, the directory name is the database name. - grouped, err := schema.GroupFilesByNamespace(rawFiles, filepath.Base(dir), environment) + grouped, ignored, err := schema.GroupFilesByNamespace(rawFiles, filepath.Base(dir), environment, ignoreNamespaces) if err != nil { - return nil, err + return nil, nil, err } // Convert schema.SchemaFiles β†’ apitypes.SchemaFiles @@ -355,7 +377,7 @@ func ReadSchemaFiles(dir string, environment string) (map[string]*apitypes.Schem for ns, nsFiles := range grouped { result[ns] = &apitypes.SchemaFiles{Files: nsFiles.Files} } - return result, nil + return result, ignored, nil } func isSchemaFile(name string) bool { diff --git a/pkg/cmd/client/client_test.go b/pkg/cmd/client/client_test.go index c1c700aa7..bcaf38c28 100644 --- a/pkg/cmd/client/client_test.go +++ b/pkg/cmd/client/client_test.go @@ -190,7 +190,7 @@ func TestReadSchemaFiles_RegularDirectories(t *testing.T) { require.NoError(t, os.MkdirAll(filepath.Join(dir, "ks_sharded"), 0o755)) require.NoError(t, os.WriteFile(filepath.Join(dir, "ks_sharded", "orders.sql"), []byte("CREATE TABLE orders (id INT)"), 0o644)) - result, err := ReadSchemaFiles(dir, "") + result, _, err := ReadSchemaFiles(dir, "", nil) require.NoError(t, err) require.Contains(t, result, "ks_unsharded") @@ -211,7 +211,7 @@ func TestReadSchemaFiles_SymlinkedDirectories(t *testing.T) { symlinkDir := filepath.Join(dir, "ks_symlink") require.NoError(t, os.Symlink(realDir, symlinkDir)) - result, err := ReadSchemaFiles(dir, "") + result, _, err := ReadSchemaFiles(dir, "", nil) require.NoError(t, err) // Both the real directory and the symlink should be read @@ -237,7 +237,7 @@ func TestReadSchemaFiles_MixedRealAndSymlinked(t *testing.T) { require.NoError(t, os.Symlink(shardedDir, filepath.Join(dir, "commerce_sharded_001"))) require.NoError(t, os.Symlink(shardedDir, filepath.Join(dir, "commerce_sharded_002"))) - result, err := ReadSchemaFiles(dir, "") + result, _, err := ReadSchemaFiles(dir, "", nil) require.NoError(t, err) // All four keyspaces should be present @@ -260,7 +260,7 @@ func TestReadSchemaFiles_SkipsNonSchemaFiles(t *testing.T) { require.NoError(t, os.WriteFile(filepath.Join(dir, "mydb", "README.md"), []byte("ignore me"), 0o644)) require.NoError(t, os.WriteFile(filepath.Join(dir, "mydb", "vschema.json"), []byte("{}"), 0o644)) - result, err := ReadSchemaFiles(dir, "") + result, _, err := ReadSchemaFiles(dir, "", nil) require.NoError(t, err) require.Contains(t, result, "mydb") @@ -268,3 +268,67 @@ func TestReadSchemaFiles_SkipsNonSchemaFiles(t *testing.T) { assert.Contains(t, result["mydb"].Files, "vschema.json") assert.NotContains(t, result["mydb"].Files, "README.md") } + +func TestReadSchemaFiles_IgnoreNamespaces(t *testing.T) { + dir := t.TempDir() + + require.NoError(t, os.MkdirAll(filepath.Join(dir, "ks_unsharded"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "ks_unsharded", "users.sql"), []byte("CREATE TABLE users (id INT)"), 0o644)) + + require.NoError(t, os.MkdirAll(filepath.Join(dir, "local_fixtures"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "local_fixtures", "widgets.sql"), []byte("CREATE TABLE widgets (id INT)"), 0o644)) + + result, removed, err := ReadSchemaFiles(dir, "", []string{"local_fixtures"}) + require.NoError(t, err) + + require.Contains(t, result, "ks_unsharded") + assert.NotContains(t, result, "local_fixtures") + assert.Equal(t, []string{"local_fixtures"}, removed) +} + +// A flat-layout schema directory has exactly one namespace β€” the directory +// name itself β€” so ignoring it removes every schema file. The plan call must +// fail with an error naming the exclusion rather than planning an empty +// desired state, which on a live database would read as "drop everything". +func TestCallPlanAPI_IgnoreSoleFlatNamespace(t *testing.T) { + dir := filepath.Join(t.TempDir(), "orders") + require.NoError(t, os.MkdirAll(dir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "users.sql"), []byte("CREATE TABLE users (id INT)"), 0o644)) + + _, ignored, err := CallPlanAPI("http://unreachable.invalid", "orders", "mysql", "development", dir, "", 0, []string{"orders"}) + require.Error(t, err) + assert.Contains(t, err.Error(), "after excluding ignored namespaces") + assert.Contains(t, err.Error(), "orders") + assert.Equal(t, []string{"orders"}, ignored) +} + +// The namespaces removed by ignore_namespaces must reach the server on the +// plan request: their files are already absent from schema_files, and the +// server needs the ignored list to refuse engine shapes that cannot honor +// the exclusion. +func TestCallPlanAPI_SendsIgnoredNamespaces(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(dir, "payments"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "payments", "users.sql"), []byte("CREATE TABLE users (id INT)"), 0o644)) + require.NoError(t, os.MkdirAll(filepath.Join(dir, "local_fixtures"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "local_fixtures", "widgets.sql"), []byte("CREATE TABLE widgets (id INT)"), 0o644)) + + var gotReq apitypes.PlanRequest + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + require.Equal(t, http.MethodPost, r.Method) + assert.Equal(t, "/api/plan", r.URL.Path) + require.NoError(t, json.NewDecoder(r.Body).Decode(&gotReq)) + + w.Header().Set("Content-Type", "application/json") + require.NoError(t, json.NewEncoder(w).Encode(apitypes.PlanResponse{})) + })) + t.Cleanup(server.Close) + + _, ignored, err := CallPlanAPI(server.URL, "orders", "mysql", "development", dir, "", 0, []string{"local_fixtures"}) + require.NoError(t, err) + + assert.Equal(t, []string{"local_fixtures"}, ignored) + assert.Equal(t, []string{"local_fixtures"}, gotReq.IgnoredNamespaces) + require.Contains(t, gotReq.SchemaFiles, "payments") + assert.NotContains(t, gotReq.SchemaFiles, "local_fixtures") +} diff --git a/pkg/cmd/commands/apply.go b/pkg/cmd/commands/apply.go index a0923263a..fd1ecfa5a 100644 --- a/pkg/cmd/commands/apply.go +++ b/pkg/cmd/commands/apply.go @@ -14,6 +14,7 @@ import ( "github.com/block/schemabot/pkg/cmd/cliname" "github.com/block/schemabot/pkg/cmd/internal/templates" "github.com/block/schemabot/pkg/ddl" + "github.com/block/schemabot/pkg/schema" "github.com/block/schemabot/pkg/state" "github.com/block/schemabot/pkg/ui" ) @@ -102,14 +103,19 @@ func (cmd *ApplyCmd) Run(g *Globals) error { // Step 1: Generate plan var planResult *apitypes.PlanResponse + var ignoredNamespaces []string err = withLoading("Generating schema change plan...", cmd.Output != OutputFormatJSON, func() error { var planErr error - planResult, planErr = client.CallPlanAPI(ep, cfg.Database, cfg.Type, cmd.Environment, cfg.SchemaDir, cmd.Repository, cmd.PullRequest) + planResult, ignoredNamespaces, planErr = client.CallPlanAPI(ep, cfg.Database, cfg.Type, cmd.Environment, cfg.SchemaDir, cmd.Repository, cmd.PullRequest, cfg.IgnoreNamespaces) return planErr }) if err != nil { return err } + if cmd.Output != OutputFormatJSON { + templates.WriteIgnoredNamespaces(ignoredNamespaces, + schema.UnmatchedIgnoreEntries(cfg.IgnoreNamespaces, cmd.Environment, ignoredNamespaces)) + } // Validate engine-specific options if cmd.SkipRevert && planResult.Engine != "" && !state.IsPlanetScaleEngine(planResult.Engine) { diff --git a/pkg/cmd/commands/common.go b/pkg/cmd/commands/common.go index 79a2260aa..d182422ab 100644 --- a/pkg/cmd/commands/common.go +++ b/pkg/cmd/commands/common.go @@ -20,6 +20,7 @@ import ( "github.com/block/schemabot/pkg/cmd/client" "github.com/block/schemabot/pkg/cmd/cliname" "github.com/block/schemabot/pkg/cmd/internal/templates" + "github.com/block/schemabot/pkg/schema" "github.com/block/schemabot/pkg/state" ) @@ -70,9 +71,12 @@ var ErrSilent = errors.New("silent error") // CLIConfig represents the schemabot.yaml configuration file for CLI commands. type CLIConfig struct { - Database string `yaml:"database"` - Type string `yaml:"type"` - SchemaDir string `yaml:"-"` // Set by LoadCLIConfig, not from YAML + Database string `yaml:"database"` + Type string `yaml:"type"` + // IgnoreNamespaces lists namespace subdirectories of the schema root that + // SchemaBot must not reconcile against the live database. + IgnoreNamespaces []string `yaml:"ignore_namespaces"` + SchemaDir string `yaml:"-"` // Set by LoadCLIConfig, not from YAML } // LoadCLIConfig loads configuration from schemabot.yaml in the given directory. @@ -102,6 +106,9 @@ func LoadCLIConfig(dir string) (*CLIConfig, error) { if cfg.Database == "" { return nil, fmt.Errorf("schemabot.yaml: database is required") } + if err := schema.ValidateIgnoreNamespaces(cfg.IgnoreNamespaces); err != nil { + return nil, fmt.Errorf("schemabot.yaml: %w", err) + } // Schema files are in the same directory as schemabot.yaml cfg.SchemaDir = dir if cfg.Type == "" { diff --git a/pkg/cmd/commands/common_test.go b/pkg/cmd/commands/common_test.go index 84143d1a1..2e9b4ab62 100644 --- a/pkg/cmd/commands/common_test.go +++ b/pkg/cmd/commands/common_test.go @@ -72,3 +72,24 @@ func TestApplyChangeCountsSummaryVSchemaOnly(t *testing.T) { func TestApplyChangeCountsSummaryEmpty(t *testing.T) { assert.Empty(t, countTableProgressChanges(nil).summary()) } + +func TestLoadCLIConfig_ParsesIgnoreNamespaces(t *testing.T) { + dir := t.TempDir() + content := "database: mydb\ntype: vitess\nignore_namespaces:\n - local_fixtures\n" + require.NoError(t, os.WriteFile(filepath.Join(dir, "schemabot.yaml"), []byte(content), 0644)) + + cfg, err := LoadCLIConfig(dir) + require.NoError(t, err) + assert.Equal(t, []string{"local_fixtures"}, cfg.IgnoreNamespaces) +} + +func TestLoadCLIConfig_RejectsIgnoreNamespacePaths(t *testing.T) { + dir := t.TempDir() + content := "database: mydb\ntype: vitess\nignore_namespaces:\n - schema/local_fixtures\n" + require.NoError(t, os.WriteFile(filepath.Join(dir, "schemabot.yaml"), []byte(content), 0644)) + + cfg, err := LoadCLIConfig(dir) + require.Error(t, err) + assert.Nil(t, cfg) + assert.Contains(t, err.Error(), "not a path") +} diff --git a/pkg/cmd/commands/onboard.go b/pkg/cmd/commands/onboard.go index f0bcc5484..0bedbc414 100644 --- a/pkg/cmd/commands/onboard.go +++ b/pkg/cmd/commands/onboard.go @@ -53,7 +53,11 @@ func (cmd *OnboardCmd) Run(g *Globals) error { if err := rewriteOnboardNamespaces(resp, cmd.Environment, cmd.TemplateEnvSuffix); err != nil { return err } - plan, err := buildOnboardWritePlan(cmd.SchemaDir, resp) + preservedIgnores, err := preservedIgnoreNamespaces(cmd.SchemaDir) + if err != nil { + return err + } + plan, err := buildOnboardWritePlan(cmd.SchemaDir, resp, preservedIgnores) if err != nil { return err } @@ -116,6 +120,26 @@ type onboardWritePlan struct { root string databaseType string files map[string]string + // ignoreNamespaces carries an existing config's ignore_namespaces through + // a rewrite, so re-onboarding does not drop the exclusions an operator + // configured, and plan verification excludes the same namespaces a real + // plan would. + ignoreNamespaces []string +} + +// preservedIgnoreNamespaces returns the ignore_namespaces of an existing +// schemabot.yaml under schemaRoot. A missing config is a fresh onboarding with +// nothing to preserve; an unreadable one is an error β€” rewriting it would +// silently drop whatever it configured. +func preservedIgnoreNamespaces(schemaRoot string) ([]string, error) { + if _, err := os.Stat(filepath.Join(schemaRoot, "schemabot.yaml")); os.IsNotExist(err) { + return nil, nil + } + cfg, err := LoadCLIConfig(schemaRoot) + if err != nil { + return nil, fmt.Errorf("read existing schemabot.yaml to preserve ignore_namespaces: %w", err) + } + return cfg.IgnoreNamespaces, nil } func onboardPullNamespaces(namespaces []string) ([]string, error) { @@ -170,7 +194,7 @@ func onboardOutputNamespace(namespace, environment string, templateEnvSuffix boo return namespace } -func buildOnboardWritePlan(schemaRoot string, resp *apitypes.PullSchemaResponse) (*onboardWritePlan, error) { +func buildOnboardWritePlan(schemaRoot string, resp *apitypes.PullSchemaResponse, ignoreNamespaces []string) (*onboardWritePlan, error) { if strings.TrimSpace(schemaRoot) == "" { return nil, fmt.Errorf("schema root is required") } @@ -188,7 +212,7 @@ func buildOnboardWritePlan(schemaRoot string, resp *apitypes.PullSchemaResponse) } root := filepath.Clean(schemaRoot) files := map[string]string{ - "schemabot.yaml": fmt.Sprintf("database: %s\ntype: %s\n", resp.Database, resp.Type), + "schemabot.yaml": onboardConfigYAML(resp.Database, string(resp.Type), ignoreNamespaces), } namespaces := make([]string, 0, len(resp.Namespaces)) @@ -223,7 +247,19 @@ func buildOnboardWritePlan(schemaRoot string, resp *apitypes.PullSchemaResponse) } } - return &onboardWritePlan{root: root, databaseType: resp.Type, files: files}, nil + return &onboardWritePlan{root: root, databaseType: resp.Type, files: files, ignoreNamespaces: ignoreNamespaces}, nil +} + +func onboardConfigYAML(database, databaseType string, ignoreNamespaces []string) string { + var b strings.Builder + fmt.Fprintf(&b, "database: %s\ntype: %s\n", database, databaseType) + if len(ignoreNamespaces) > 0 { + b.WriteString("ignore_namespaces:\n") + for _, ns := range ignoreNamespaces { + fmt.Fprintf(&b, " - %s\n", ns) + } + } + return b.String() } func validateRelativePathPart(kind, value string) error { @@ -371,7 +407,7 @@ func verifyOnboardPlan(endpoint, database, environment string, plan *onboardWrit var planResult *apitypes.PlanResponse err := withLoading("Verifying pulled schema...", true, func() error { var planErr error - planResult, planErr = client.CallPlanAPI(endpoint, database, plan.databaseType, environment, plan.root, "", 0) + planResult, _, planErr = client.CallPlanAPI(endpoint, database, plan.databaseType, environment, plan.root, "", 0, plan.ignoreNamespaces) return planErr }) if err != nil { diff --git a/pkg/cmd/commands/onboard_test.go b/pkg/cmd/commands/onboard_test.go index 46d13d4b3..0a017a413 100644 --- a/pkg/cmd/commands/onboard_test.go +++ b/pkg/cmd/commands/onboard_test.go @@ -27,7 +27,7 @@ func TestBuildOnboardWritePlanWritesConfigAndNamespaceFiles(t *testing.T) { }, }, }, - }) + }, nil) require.NoError(t, err) require.NoError(t, plan.checkConflicts(false)) require.NoError(t, plan.write()) @@ -62,7 +62,7 @@ func TestBuildOnboardWritePlanWritesVitessKeyspaceArtifacts(t *testing.T) { }, }, }, - }) + }, nil) require.NoError(t, err) require.NoError(t, plan.checkConflicts(false)) require.NoError(t, plan.write()) @@ -80,6 +80,58 @@ func TestBuildOnboardWritePlanWritesVitessKeyspaceArtifacts(t *testing.T) { assert.JSONEq(t, "{\"sharded\":true}", string(vschema)) } +// Re-onboarding over an existing schema root must not drop the +// ignore_namespaces an operator configured: the rewritten schemabot.yaml +// carries the entries forward and the plan verification excludes the same +// namespaces a real plan would. +func TestBuildOnboardWritePlanPreservesIgnoreNamespaces(t *testing.T) { + root := t.TempDir() + plan, err := buildOnboardWritePlan(root, &apitypes.PullSchemaResponse{ + Database: "orders", + Type: "mysql", + Environment: "production", + TableCount: 1, + Namespaces: map[string]*apitypes.PulledNamespace{ + "orders": {Tables: map[string]string{"users": "CREATE TABLE `users` (`id` bigint NOT NULL);\n"}}, + }, + }, []string{"local_fixtures", "fixtures_$ENV"}) + require.NoError(t, err) + require.NoError(t, plan.write()) + + config, err := os.ReadFile(filepath.Join(root, "schemabot.yaml")) + require.NoError(t, err) + assert.Equal(t, "database: orders\ntype: mysql\nignore_namespaces:\n - local_fixtures\n - fixtures_$ENV\n", string(config)) + assert.Equal(t, []string{"local_fixtures", "fixtures_$ENV"}, plan.ignoreNamespaces) +} + +func TestPreservedIgnoreNamespaces(t *testing.T) { + t.Run("missing config is a fresh onboarding", func(t *testing.T) { + ignores, err := preservedIgnoreNamespaces(t.TempDir()) + require.NoError(t, err) + assert.Nil(t, ignores) + }) + + t.Run("existing config's entries are preserved", func(t *testing.T) { + root := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(root, "schemabot.yaml"), + []byte("database: orders\ntype: mysql\nignore_namespaces:\n - local_fixtures\n"), 0o644)) + + ignores, err := preservedIgnoreNamespaces(root) + require.NoError(t, err) + assert.Equal(t, []string{"local_fixtures"}, ignores) + }) + + t.Run("unreadable config is an error, not a silent drop", func(t *testing.T) { + root := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(root, "schemabot.yaml"), + []byte(": not yaml"), 0o644)) + + _, err := preservedIgnoreNamespaces(root) + require.Error(t, err) + assert.Contains(t, err.Error(), "preserve ignore_namespaces") + }) +} + func TestOnboardPullNamespacesUseConcreteLiveNamespaces(t *testing.T) { pullNamespaces, err := onboardPullNamespaces([]string{"orders_production", "orders_audit_production"}) require.NoError(t, err) @@ -131,7 +183,7 @@ func TestOnboardWritePlanRefusesExistingFilesWithoutForce(t *testing.T) { Namespaces: map[string]*apitypes.PulledNamespace{ "orders": {Tables: map[string]string{"users": "CREATE TABLE `users` (`id` bigint NOT NULL);\n"}}, }, - }) + }, nil) require.NoError(t, err) err = plan.checkConflicts(false) @@ -150,7 +202,7 @@ func TestBuildOnboardWritePlanRejectsUnsafeResponsePaths(t *testing.T) { Namespaces: map[string]*apitypes.PulledNamespace{ "orders": {Tables: map[string]string{"../users": "CREATE TABLE `users` (`id` bigint NOT NULL);\n"}}, }, - }) + }, nil) require.Error(t, err) assert.Contains(t, err.Error(), "table") } @@ -205,7 +257,7 @@ func TestBuildOnboardWritePlanRejectsInvalidPullResponse(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - plan, err := buildOnboardWritePlan(tt.schemaRoot, tt.resp) + plan, err := buildOnboardWritePlan(tt.schemaRoot, tt.resp, nil) require.Error(t, err) assert.Nil(t, plan) assert.Contains(t, err.Error(), tt.want) @@ -300,7 +352,7 @@ func TestOnboardWritePlanStrayFiles(t *testing.T) { Namespaces: map[string]*apitypes.PulledNamespace{ "orders": {Tables: map[string]string{"users": "CREATE TABLE `users` (`id` bigint NOT NULL);\n"}}, }, - }) + }, nil) require.NoError(t, err) // Namespace directory absent (dry run before any write): nothing to scan. @@ -337,7 +389,7 @@ func TestOnboardWritePlanStrayFilesFlagsVSchemaForVitess(t *testing.T) { Namespaces: map[string]*apitypes.PulledNamespace{ "orders": {Tables: map[string]string{"users": "CREATE TABLE `users` (`id` bigint NOT NULL);\n"}}, }, - }) + }, nil) require.NoError(t, err) require.NoError(t, plan.write()) diff --git a/pkg/cmd/commands/plan.go b/pkg/cmd/commands/plan.go index 383f5eb37..ee96b20ad 100644 --- a/pkg/cmd/commands/plan.go +++ b/pkg/cmd/commands/plan.go @@ -16,6 +16,7 @@ import ( "github.com/block/schemabot/pkg/cmd/cliname" "github.com/block/schemabot/pkg/cmd/internal/templates" "github.com/block/schemabot/pkg/ddl" + "github.com/block/schemabot/pkg/schema" "github.com/block/schemabot/pkg/state" ) @@ -79,11 +80,12 @@ func (cmd *PlanCmd) Run(g *Globals) error { // Collect results for all environments allResults := make(map[string]*apitypes.PlanResponse) + ignoredByEnv := make(map[string][]string) for _, env := range environments { var result *apitypes.PlanResponse err := withLoading("Generating schema change plan...", !cmd.JSON, func() error { var planErr error - result, planErr = client.CallPlanAPI(ep, cfg.Database, cfg.Type, env, cfg.SchemaDir, cmd.Repository, cmd.PullRequest) + result, ignoredByEnv[env], planErr = client.CallPlanAPI(ep, cfg.Database, cfg.Type, env, cfg.SchemaDir, cmd.Repository, cmd.PullRequest, cfg.IgnoreNamespaces) return planErr }) if err != nil { @@ -102,6 +104,20 @@ func (cmd *PlanCmd) Run(g *Globals) error { return writeJSON(allResults) } + // Disclose config-driven exclusions once per distinct resolution β€” the + // lists only differ between environments when entries use $ENV. + disclosed := make(map[string]bool) + for _, env := range environments { + ignored := ignoredByEnv[env] + unmatched := schema.UnmatchedIgnoreEntries(cfg.IgnoreNamespaces, env, ignored) + key := strings.Join(ignored, ",") + "|" + strings.Join(unmatched, ",") + if disclosed[key] { + continue + } + disclosed[key] = true + templates.WriteIgnoredNamespaces(ignored, unmatched) + } + // Human-readable output for all environments outputMultiEnvPlanResult(allResults, cfg.Database, cfg.SchemaDir) return nil diff --git a/pkg/cmd/internal/templates/plan.go b/pkg/cmd/internal/templates/plan.go index 3113fb138..de12eb7ec 100644 --- a/pkg/cmd/internal/templates/plan.go +++ b/pkg/cmd/internal/templates/plan.go @@ -458,6 +458,23 @@ func WriteErrors(errors []string) { fmt.Println() } +// WriteIgnoredNamespaces disclosure: the plan was built from a deliberately +// partial desired state, so a reader can distinguish "this namespace has no +// changes" from "this namespace was withheld by config". Unmatched entries are +// configured exclusions that removed nothing (typo, case mismatch, or stale +// entry) β€” the namespaces they name are fully reconciled. +func WriteIgnoredNamespaces(ignored, unmatched []string) { + if len(ignored) > 0 { + fmt.Printf("ℹ️ Namespaces excluded by ignore_namespaces: %s\n", strings.Join(ignored, ", ")) + } + for _, entry := range unmatched { + fmt.Printf("⚠️ ignore_namespaces entry %q matched no namespace and excluded nothing\n", entry) + } + if len(ignored) > 0 || len(unmatched) > 0 { + fmt.Println() + } +} + // UnsafeChange is a type alias for the shared unsafe change type. type UnsafeChange = apitypes.UnsafeChange diff --git a/pkg/cmd/internal/templates/preview.go b/pkg/cmd/internal/templates/preview.go index 95b9e8fa7..95e05d2c4 100644 --- a/pkg/cmd/internal/templates/preview.go +++ b/pkg/cmd/internal/templates/preview.go @@ -111,6 +111,7 @@ const ( // Comment template previews (GitHub PR comments) PreviewCommentPlan PreviewType = "comment_plan" // Plan comment with DDL changes + lint violations + PreviewCommentPlanIgnoredNamespaces PreviewType = "comment_plan_ignored_namespaces" // Plan with namespaces withheld by ignore_namespaces PreviewCommentPlanBlocked PreviewType = "comment_plan_blocked" // Plan with a statement the engine refuses (blocked verdict) PreviewCommentPlanDirect PreviewType = "comment_plan_direct" // Locked plan with a statement routed to direct execution (direct verdict) PreviewCommentApplyBlockedRejected PreviewType = "comment_apply_blocked_rejected" // Apply rejected: plan contains engine-blocked statements diff --git a/pkg/cmd/internal/templates/preview_comment.go b/pkg/cmd/internal/templates/preview_comment.go index e3b40f897..ae90b6419 100644 --- a/pkg/cmd/internal/templates/preview_comment.go +++ b/pkg/cmd/internal/templates/preview_comment.go @@ -40,6 +40,7 @@ func previewCommentAllOutput() { fn func() }{ {"PLAN COMMENT", func() { fmt.Print(webhooktemplates.PreviewCommentPlan()) }}, + {"PLAN COMMENT (IGNORED NAMESPACES)", func() { fmt.Print(webhooktemplates.PreviewCommentPlanIgnoredNamespaces()) }}, {"PLAN COMMENT (MANY LINT WARNINGS)", func() { fmt.Print(webhooktemplates.PreviewCommentPlanManyLintWarnings()) }}, {"PLAN COMMENT (ENGINE-BLOCKED CHANGE)", func() { fmt.Print(webhooktemplates.PreviewCommentPlanBlocked()) }}, {"PLAN COMMENT (DIRECT-EXECUTION CHANGE)", func() { fmt.Print(webhooktemplates.PreviewCommentPlanDirect()) }}, @@ -131,6 +132,7 @@ func previewCommentPlanAllOutput() { fn func() }{ {"MYSQL PLAN", func() { fmt.Print(webhooktemplates.PreviewCommentPlan()) }}, + {"MYSQL PLAN (IGNORED NAMESPACES)", func() { fmt.Print(webhooktemplates.PreviewCommentPlanIgnoredNamespaces()) }}, {"MYSQL PLAN (MANY LINT WARNINGS)", func() { fmt.Print(webhooktemplates.PreviewCommentPlanManyLintWarnings()) }}, {"MYSQL PLAN (ENGINE-BLOCKED CHANGE)", func() { fmt.Print(webhooktemplates.PreviewCommentPlanBlocked()) }}, {"MYSQL PLAN (DIRECT-EXECUTION CHANGE)", func() { fmt.Print(webhooktemplates.PreviewCommentPlanDirect()) }}, diff --git a/pkg/cmd/internal/templates/preview_dispatch.go b/pkg/cmd/internal/templates/preview_dispatch.go index 19c92fdc4..b78174922 100644 --- a/pkg/cmd/internal/templates/preview_dispatch.go +++ b/pkg/cmd/internal/templates/preview_dispatch.go @@ -116,6 +116,8 @@ func PreviewCLIOutput(previewType PreviewType) { // Comment template previews case PreviewCommentPlan: fmt.Print(webhooktemplates.PreviewCommentPlan()) + case PreviewCommentPlanIgnoredNamespaces: + fmt.Print(webhooktemplates.PreviewCommentPlanIgnoredNamespaces()) case PreviewCommentPlanBlocked: fmt.Print(webhooktemplates.PreviewCommentPlanBlocked()) case PreviewCommentPlanDirect: diff --git a/pkg/github/client_test.go b/pkg/github/client_test.go index 9b703bb48..8cd0c89fc 100644 --- a/pkg/github/client_test.go +++ b/pkg/github/client_test.go @@ -390,7 +390,7 @@ func TestFetchSchemaFilesOptimizedFollowsSymlinkedNamespaces(t *testing.T) { } // Grouping keys each file under its symlink (keyspace) name. - grouped, err := groupFilesByNamespace(files, "schema", "") + grouped, _, err := groupFilesByNamespace(files, "schema", "", nil) require.NoError(t, err) require.Contains(t, grouped, "shard_001") require.Contains(t, grouped, "shard_002") diff --git a/pkg/github/config.go b/pkg/github/config.go index 06bd7de22..a86aa974b 100644 --- a/pkg/github/config.go +++ b/pkg/github/config.go @@ -9,6 +9,8 @@ import ( "strings" "gopkg.in/yaml.v3" + + "github.com/block/schemabot/pkg/schema" ) // DatabaseType represents the type of database backend. @@ -27,6 +29,11 @@ type SchemabotConfig struct { Database string `yaml:"database" json:"database"` Name string `yaml:"name" json:"name"` Type DatabaseType `yaml:"type,omitempty" json:"type,omitempty"` + // IgnoreNamespaces lists namespace subdirectories of the schema root that + // SchemaBot must not reconcile against the live database β€” for example a + // keyspace that only exists in local test infrastructure. Ignored + // namespaces are excluded from plans, applies, and checks. + IgnoreNamespaces []string `yaml:"ignore_namespaces,omitempty" json:"ignore_namespaces,omitempty"` } // GetType returns the database type. Type is always set β€” FetchConfig rejects empty values. @@ -107,6 +114,9 @@ func (ic *InstallationClient) FetchConfig(ctx context.Context, repo, configPath, default: return nil, fmt.Errorf("invalid schemabot.yaml at %s: type must be 'vitess', 'mysql', 'strata', or 'postgres', got '%s'", configPath, config.Type) } + if err := schema.ValidateIgnoreNamespaces(config.IgnoreNamespaces); err != nil { + return nil, fmt.Errorf("invalid schemabot.yaml at %s: %w", configPath, err) + } return &config, nil } diff --git a/pkg/github/config_test.go b/pkg/github/config_test.go index 4e79a0219..b39d7547e 100644 --- a/pkg/github/config_test.go +++ b/pkg/github/config_test.go @@ -37,6 +37,21 @@ environments: assert.Contains(t, err.Error(), "field environments not found") } +func TestSchemabotConfigParsesIgnoreNamespaces(t *testing.T) { + yamlData := ` +database: testdb +type: vitess +ignore_namespaces: + - local_fixtures + - fixtures_$ENV +` + var config SchemabotConfig + decoder := yaml.NewDecoder(strings.NewReader(yamlData)) + decoder.KnownFields(true) + require.NoError(t, decoder.Decode(&config)) + assert.Equal(t, []string{"local_fixtures", "fixtures_$ENV"}, config.IgnoreNamespaces) +} + func TestHasSchemaInputFiles(t *testing.T) { t.Parallel() diff --git a/pkg/github/schema.go b/pkg/github/schema.go index 1652b7a5f..5af814a35 100644 --- a/pkg/github/schema.go +++ b/pkg/github/schema.go @@ -26,6 +26,12 @@ type SchemaRequestResult struct { // object and its target so retargeting the environment cannot go unnoticed. SchemaLinkPath string HeadSHA string // Commit SHA used to fetch schema files + // IgnoredNamespaces lists the namespaces that were actually removed from + // SchemaFiles by the config's ignore_namespaces, resolved for the + // environment and sorted. Surfaces the exclusion on the PR (plan comment) + // and travels with the plan request so the data plane can refuse engine + // shapes that cannot honor it. + IgnoredNamespaces []string } // EnvironmentValidator verifies that a discovered database can be used with @@ -81,23 +87,41 @@ func (ic *InstallationClient) CreateSchemaRequestForConfig(ctx context.Context, } // Group files by keyspace/namespace - schemaFiles, err := groupFilesByNamespace(files, schemaRoot, environment) + schemaFiles, ignoredNamespaces, err := groupFilesByNamespace(files, schemaRoot, environment, config.IgnoreNamespaces) if err != nil { return nil, err } + if len(ignoredNamespaces) > 0 { + ic.logger.Info("excluding ignored namespaces from schema request", + "repo", repo, "pr", pr, "database", config.Database, "environment", environment, + "schema_root", schemaRoot, "ignored_namespaces", ignoredNamespaces) + } + // A configured entry that removed nothing is a typo, a case mismatch, or a + // stale entry for a deleted directory β€” the namespace it names is being + // fully reconciled, so surface it rather than letting the config imply an + // exclusion that is not happening. + if unmatched := schema.UnmatchedIgnoreEntries(config.IgnoreNamespaces, environment, ignoredNamespaces); len(unmatched) > 0 { + ic.logger.Warn("ignore_namespaces entries matched no namespace and excluded nothing", + "repo", repo, "pr", pr, "database", config.Database, "environment", environment, + "schema_root", schemaRoot, "unmatched_entries", unmatched) + } if len(schemaFiles) == 0 { + if len(ignoredNamespaces) > 0 { + return nil, fmt.Errorf("no schema files found under %s for environment %q after excluding ignored namespaces %v", schemaRoot, environment, ignoredNamespaces) + } return nil, fmt.Errorf("no schema files found under %s for environment %q", schemaRoot, environment) } return &SchemaRequestResult{ - Database: config.Database, - Type: string(config.GetType()), - SchemaFiles: schemaFiles, - Repository: repo, - PullRequest: pr, - SchemaPath: schemaRoot, - SchemaLinkPath: schemaLinkPath, - HeadSHA: prInfo.HeadSHA, + Database: config.Database, + Type: string(config.GetType()), + SchemaFiles: schemaFiles, + Repository: repo, + PullRequest: pr, + SchemaPath: schemaRoot, + SchemaLinkPath: schemaLinkPath, + HeadSHA: prInfo.HeadSHA, + IgnoredNamespaces: ignoredNamespaces, }, nil } @@ -183,7 +207,9 @@ func schemaPathWithinDirectory(directory, candidate string) bool { // Files in namespace subdirectories (schema/namespace/table.sql) use the // subdirectory name as the namespace. Flat files (schema/table.sql) use the // schema directory name as the namespace (the MySQL database name). -func groupFilesByNamespace(files []GitHubFile, schemaPath, environment string) (map[string]*ternv1.SchemaFiles, error) { +// Namespaces listed in ignoreNamespaces are excluded from the result; the +// second return value lists the namespace keys actually removed, sorted. +func groupFilesByNamespace(files []GitHubFile, schemaPath, environment string, ignoreNamespaces []string) (map[string]*ternv1.SchemaFiles, []string, error) { // Build relativePath β†’ content map for the shared helper. // file.Path is the full path (e.g., "schema/payments/transactions.sql"), // so trimming schemaPath+"/" gives the relative path ("payments/transactions.sql" @@ -193,14 +219,14 @@ func groupFilesByNamespace(files []GitHubFile, schemaPath, environment string) ( for _, file := range files { relPath, ok := strings.CutPrefix(file.Path, prefix) if !ok { - return nil, fmt.Errorf("file path %q does not start with schema path %q", file.Path, prefix) + return nil, nil, fmt.Errorf("file path %q does not start with schema path %q", file.Path, prefix) } rawFiles[relPath] = file.Content } - grouped, err := schema.GroupFilesByNamespace(rawFiles, path.Base(schemaPath), environment) + grouped, removed, err := schema.GroupFilesByNamespace(rawFiles, path.Base(schemaPath), environment, ignoreNamespaces) if err != nil { - return nil, err + return nil, nil, err } // Convert schema.SchemaFiles β†’ ternv1.SchemaFiles @@ -208,5 +234,5 @@ func groupFilesByNamespace(files []GitHubFile, schemaPath, environment string) ( for ns, nsFiles := range grouped { result[ns] = &ternv1.SchemaFiles{Files: nsFiles.Files} } - return result, nil + return result, removed, nil } diff --git a/pkg/github/schema_test.go b/pkg/github/schema_test.go index c742bef75..63fd6ea47 100644 --- a/pkg/github/schema_test.go +++ b/pkg/github/schema_test.go @@ -21,7 +21,7 @@ func TestGroupFilesByNamespace_FlatLayout_UsesDirectoryName(t *testing.T) { {Path: "schema/payments/refunds.sql", Name: "refunds.sql", Content: "CREATE TABLE refunds (...);"}, } - result, err := groupFilesByNamespace(files, "schema/payments", "development") + result, _, err := groupFilesByNamespace(files, "schema/payments", "development", nil) require.NoError(t, err) require.Len(t, result, 1) @@ -45,7 +45,7 @@ func TestGroupFilesByNamespace_FlatLayout_IssueScenario(t *testing.T) { {Path: "schema/aurora_coffeeshop_exemplar/customers.sql", Name: "customers.sql", Content: "CREATE TABLE customers (...);"}, } - result, err := groupFilesByNamespace(files, "schema/aurora_coffeeshop_exemplar", "development") + result, _, err := groupFilesByNamespace(files, "schema/aurora_coffeeshop_exemplar", "development", nil) require.NoError(t, err) require.Len(t, result, 1) @@ -62,7 +62,7 @@ func TestGroupFilesByNamespace_FlatLayout_TopLevelSchemaDir(t *testing.T) { {Path: "schema/orders.sql", Name: "orders.sql", Content: "CREATE TABLE orders (...);"}, } - result, err := groupFilesByNamespace(files, "schema", "development") + result, _, err := groupFilesByNamespace(files, "schema", "development", nil) require.NoError(t, err) require.Len(t, result, 1) @@ -88,7 +88,7 @@ func TestGroupFilesByNamespace_SubdirLayout_MultipleNamespaces(t *testing.T) { {Path: "schema/payments_audit/audit_log.sql", Name: "audit_log.sql", Content: "CREATE TABLE audit_log (...);"}, } - result, err := groupFilesByNamespace(files, "schema", "development") + result, _, err := groupFilesByNamespace(files, "schema", "development", nil) require.NoError(t, err) require.Len(t, result, 2) @@ -106,7 +106,7 @@ func TestGroupFilesByNamespace_SubdirLayout_Monorepo(t *testing.T) { {Path: "payments-service/mysql/schema/payments_audit/audit_log.sql", Name: "audit_log.sql", Content: "CREATE TABLE audit_log (...);"}, } - result, err := groupFilesByNamespace(files, "payments-service/mysql/schema", "development") + result, _, err := groupFilesByNamespace(files, "payments-service/mysql/schema", "development", nil) require.NoError(t, err) require.Len(t, result, 2) @@ -123,7 +123,7 @@ func TestGroupFilesByNamespace_SubdirLayout_VitessKeyspaces(t *testing.T) { {Path: "schema/customers/users.sql", Name: "users.sql", Content: "CREATE TABLE users (...);"}, } - result, err := groupFilesByNamespace(files, "schema", "development") + result, _, err := groupFilesByNamespace(files, "schema", "development", nil) require.NoError(t, err) require.Len(t, result, 2) @@ -142,7 +142,7 @@ func TestGroupFilesByNamespace_MixedLayout_Rejected(t *testing.T) { {Path: "schema/payments/transactions.sql", Name: "transactions.sql", Content: "CREATE TABLE transactions (...);"}, } - _, err := groupFilesByNamespace(files, "schema", "development") + _, _, err := groupFilesByNamespace(files, "schema", "development", nil) require.Error(t, err) assert.Contains(t, err.Error(), "both flat files and namespace subdirectories") diff --git a/pkg/proto/tern.proto b/pkg/proto/tern.proto index 11f5752d9..df9653a06 100644 --- a/pkg/proto/tern.proto +++ b/pkg/proto/tern.proto @@ -435,6 +435,12 @@ message PlanRequest { string head_sha = 9; // Repo-relative schema directory discovered by SchemaBot from the PR. string schema_path = 10; + // Namespaces the caller removed from schema_files per the config's + // ignore_namespaces, resolved for the environment. The data plane refuses + // engine shapes that diff the whole target as one unit (a database-scoped + // MySQL DSN), where a withheld namespace's live tables would otherwise be + // planned as drops. + repeated string ignored_namespaces = 11; } // TableChange represents a DDL change to a table. diff --git a/pkg/proto/ternv1/tern.pb.go b/pkg/proto/ternv1/tern.pb.go index 910503bd0..4d6a1211f 100644 --- a/pkg/proto/ternv1/tern.pb.go +++ b/pkg/proto/ternv1/tern.pb.go @@ -1042,9 +1042,15 @@ type PlanRequest struct { // without a PR context). HeadSha string `protobuf:"bytes,9,opt,name=head_sha,json=headSha,proto3" json:"head_sha,omitempty"` // Repo-relative schema directory discovered by SchemaBot from the PR. - SchemaPath string `protobuf:"bytes,10,opt,name=schema_path,json=schemaPath,proto3" json:"schema_path,omitempty"` - unknownFields protoimpl.UnknownFields - sizeCache protoimpl.SizeCache + SchemaPath string `protobuf:"bytes,10,opt,name=schema_path,json=schemaPath,proto3" json:"schema_path,omitempty"` + // Namespaces the caller removed from schema_files per the config's + // ignore_namespaces, resolved for the environment. The data plane refuses + // engine shapes that diff the whole target as one unit (a database-scoped + // MySQL DSN), where a withheld namespace's live tables would otherwise be + // planned as drops. + IgnoredNamespaces []string `protobuf:"bytes,11,rep,name=ignored_namespaces,json=ignoredNamespaces,proto3" json:"ignored_namespaces,omitempty"` + unknownFields protoimpl.UnknownFields + sizeCache protoimpl.SizeCache } func (x *PlanRequest) Reset() { @@ -1140,6 +1146,13 @@ func (x *PlanRequest) GetSchemaPath() string { return "" } +func (x *PlanRequest) GetIgnoredNamespaces() []string { + if x != nil { + return x.IgnoredNamespaces + } + return nil +} + // TableChange represents a DDL change to a table. type TableChange struct { state protoimpl.MessageState `protogen:"open.v1"` @@ -3747,7 +3760,7 @@ const file_tern_proto_rawDesc = "" + "tableCount\x1aW\n" + "\x0fNamespacesEntry\x12\x10\n" + "\x03key\x18\x01 \x01(\tR\x03key\x12.\n" + - "\x05value\x18\x02 \x01(\v2\x18.tern.v1.PulledNamespaceR\x05value:\x028\x01\"\x9c\x03\n" + + "\x05value\x18\x02 \x01(\v2\x18.tern.v1.PulledNamespaceR\x05value:\x028\x01\"\xcb\x03\n" + "\vPlanRequest\x12\x1a\n" + "\bdatabase\x18\x01 \x01(\tR\bdatabase\x12\x12\n" + "\x04type\x18\x02 \x01(\tR\x04type\x12H\n" + @@ -3761,7 +3774,8 @@ const file_tern_proto_rawDesc = "" + "\bhead_sha\x18\t \x01(\tR\aheadSha\x12\x1f\n" + "\vschema_path\x18\n" + " \x01(\tR\n" + - "schemaPath\x1aT\n" + + "schemaPath\x12-\n" + + "\x12ignored_namespaces\x18\v \x03(\tR\x11ignoredNamespaces\x1aT\n" + "\x10SchemaFilesEntry\x12\x10\n" + "\x03key\x18\x01 \x01(\tR\x03key\x12*\n" + "\x05value\x18\x02 \x01(\v2\x14.tern.v1.SchemaFilesR\x05value:\x028\x01J\x04\b\a\x10\b\"\x9c\x02\n" + diff --git a/pkg/schema/namespace.go b/pkg/schema/namespace.go index a930c350d..67d830c11 100644 --- a/pkg/schema/namespace.go +++ b/pkg/schema/namespace.go @@ -3,6 +3,7 @@ package schema import ( "fmt" "path" + "sort" "strings" ) @@ -44,7 +45,22 @@ import ( // This allows a single directory like "bikeshare_$ENV/" to resolve to // "bikeshare_staging" or "bikeshare_production" depending on the target. // If environment is empty, "$ENV" is left as-is. -func GroupFilesByNamespace(files map[string]string, defaultNamespace string, environment string) (SchemaFiles, error) { +// +// ignoreNamespaces lists namespaces to exclude from the result β€” schema +// directories that exist in the repository but must not be reconciled against +// the live database (for example a keyspace only used by local test +// infrastructure). Entries receive the same $ENV substitution as directory +// names and are matched against the post-substitution namespace keys. Layout +// validation still sees ignored directories: a mixed flat/subdirectory layout +// is rejected even when the subdirectories are all ignored. +// +// The second return value lists the namespace keys that were actually removed +// by ignoreNamespaces, sorted. An entry that matches no namespace removes +// nothing (matching is exact and case-sensitive); callers that report +// exclusions must report the removed keys, and should warn when a configured +// entry is absent from them so a typo or stale entry is visible instead of +// silently reconciling the namespace it was meant to exclude. +func GroupFilesByNamespace(files map[string]string, defaultNamespace string, environment string, ignoreNamespaces []string) (SchemaFiles, []string, error) { result := make(SchemaFiles) var hasFlatFile, hasNamespacedFile bool @@ -77,8 +93,76 @@ func GroupFilesByNamespace(files map[string]string, defaultNamespace string, env // Reject mixed flat + namespaced files if hasFlatFile && hasNamespacedFile { - return nil, fmt.Errorf("schema directory has both flat files and namespace subdirectories β€” use one layout or the other") + return nil, nil, fmt.Errorf("schema directory has both flat files and namespace subdirectories β€” use one layout or the other") + } + + var removed []string + for _, ignored := range ResolveIgnoreNamespaces(ignoreNamespaces, environment) { + if _, ok := result[ignored]; !ok { + continue + } + delete(result, ignored) + removed = append(removed, ignored) } + sort.Strings(removed) - return result, nil + return result, removed, nil +} + +// ResolveIgnoreNamespaces applies the same $ENV substitution to +// ignore_namespaces entries that GroupFilesByNamespace applies to namespace +// directory names, returning the configured entries as they will be matched +// against namespace keys for the environment. Resolution does not check +// existence β€” an entry may match no namespace; GroupFilesByNamespace reports +// which entries actually removed one. +func ResolveIgnoreNamespaces(namespaces []string, environment string) []string { + if len(namespaces) == 0 { + return nil + } + resolved := make([]string, len(namespaces)) + for i, ns := range namespaces { + if environment != "" { + ns = strings.ReplaceAll(ns, "$ENV", environment) + } + resolved[i] = ns + } + return resolved +} + +// UnmatchedIgnoreEntries returns the resolved ignore_namespaces entries that +// removed no namespace during grouping β€” a typo, a case mismatch, or a stale +// entry for a directory that no longer exists. The namespaces such entries +// name are fully reconciled, so callers should warn with the returned values +// rather than letting the config imply an exclusion that is not happening. +func UnmatchedIgnoreEntries(configured []string, environment string, removed []string) []string { + removedSet := make(map[string]bool, len(removed)) + for _, ns := range removed { + removedSet[ns] = true + } + var unmatched []string + for _, ns := range ResolveIgnoreNamespaces(configured, environment) { + if !removedSet[ns] { + unmatched = append(unmatched, ns) + } + } + return unmatched +} + +// ValidateIgnoreNamespaces rejects ignore_namespaces entries that cannot match +// a namespace subdirectory of the schema root: blank entries, entries padded +// with whitespace (namespace keys are never padded, so such an entry would +// silently exclude nothing), and entries with path separators. +func ValidateIgnoreNamespaces(namespaces []string) error { + for _, ns := range namespaces { + if strings.TrimSpace(ns) == "" { + return fmt.Errorf("ignore_namespaces entries must not be blank") + } + if ns != strings.TrimSpace(ns) { + return fmt.Errorf("ignore_namespaces entry %q must not have leading or trailing whitespace", ns) + } + if strings.ContainsAny(ns, `/\`) { + return fmt.Errorf("ignore_namespaces entry %q must be a namespace name, not a path", ns) + } + } + return nil } diff --git a/pkg/schema/namespace_test.go b/pkg/schema/namespace_test.go index da8a188c9..0df849a09 100644 --- a/pkg/schema/namespace_test.go +++ b/pkg/schema/namespace_test.go @@ -20,7 +20,7 @@ func TestGroupFilesByNamespace_FlatLayout_UsesDirectoryName(t *testing.T) { "customers.sql": "CREATE TABLE customers (...);", } - result, err := GroupFilesByNamespace(files, "aurora_coffeeshop_exemplar", "development") + result, _, err := GroupFilesByNamespace(files, "aurora_coffeeshop_exemplar", "development", nil) require.NoError(t, err) require.Len(t, result, 1) @@ -39,7 +39,7 @@ func TestGroupFilesByNamespace_FlatLayout_SkipsNonSchemaFiles(t *testing.T) { ".gitkeep": "", } - result, err := GroupFilesByNamespace(files, "myapp", "development") + result, _, err := GroupFilesByNamespace(files, "myapp", "development", nil) require.NoError(t, err) require.Len(t, result, 1) assert.Len(t, result["myapp"].Files, 1) @@ -53,7 +53,7 @@ func TestGroupFilesByNamespace_FlatLayout_IncludesVSchemaJSON(t *testing.T) { "vschema.json": `{"sharded": true}`, } - result, err := GroupFilesByNamespace(files, "commerce", "development") + result, _, err := GroupFilesByNamespace(files, "commerce", "development", nil) require.NoError(t, err) require.Len(t, result, 1) assert.Len(t, result["commerce"].Files, 2) @@ -76,7 +76,7 @@ func TestGroupFilesByNamespace_SubdirLayout_UsesSubdirNames(t *testing.T) { "payments_audit/audit_log.sql": "CREATE TABLE audit_log (...);", } - result, err := GroupFilesByNamespace(files, "ignored_because_subdirs_exist", "development") + result, _, err := GroupFilesByNamespace(files, "ignored_because_subdirs_exist", "development", nil) require.NoError(t, err) require.Len(t, result, 2) assert.Contains(t, result, "payments") @@ -93,7 +93,7 @@ func TestGroupFilesByNamespace_SubdirLayout_VSchemaInSubdir(t *testing.T) { "customers/users.sql": "CREATE TABLE users (...);", } - result, err := GroupFilesByNamespace(files, "ignored", "development") + result, _, err := GroupFilesByNamespace(files, "ignored", "development", nil) require.NoError(t, err) require.Len(t, result, 2) assert.Len(t, result["commerce"].Files, 2) @@ -110,7 +110,7 @@ func TestGroupFilesByNamespace_MixedLayout_Rejected(t *testing.T) { "payments/transactions.sql": "CREATE TABLE transactions (...);", } - _, err := GroupFilesByNamespace(files, "mydb", "development") + _, _, err := GroupFilesByNamespace(files, "mydb", "development", nil) require.Error(t, err) assert.Contains(t, err.Error(), "both flat files and namespace subdirectories") } @@ -123,7 +123,7 @@ func TestGroupFilesByNamespace_SubdirLayout_NameMatchesDefault(t *testing.T) { "other/items.sql": "CREATE TABLE items (...);", } - result, err := GroupFilesByNamespace(files, "schema", "development") + result, _, err := GroupFilesByNamespace(files, "schema", "development", nil) require.NoError(t, err) require.Len(t, result, 2) assert.Contains(t, result, "schema") @@ -133,7 +133,7 @@ func TestGroupFilesByNamespace_SubdirLayout_NameMatchesDefault(t *testing.T) { // Edge cases. func TestGroupFilesByNamespace_EmptyInput(t *testing.T) { - result, err := GroupFilesByNamespace(map[string]string{}, "mydb", "development") + result, _, err := GroupFilesByNamespace(map[string]string{}, "mydb", "development", nil) require.NoError(t, err) assert.Empty(t, result) } @@ -145,7 +145,7 @@ func TestGroupFilesByNamespace_OnlyNonSchemaFiles(t *testing.T) { "README.md": "# docs", } - result, err := GroupFilesByNamespace(files, "myapp", "development") + result, _, err := GroupFilesByNamespace(files, "myapp", "development", nil) require.NoError(t, err) assert.Empty(t, result) } @@ -160,7 +160,7 @@ func TestGroupFilesByNamespace_EnvSubstitution_SubdirLayout(t *testing.T) { "bikeshare_$ENV/stations.sql": "CREATE TABLE stations (...);", } - result, err := GroupFilesByNamespace(files, "ignored", "staging") + result, _, err := GroupFilesByNamespace(files, "ignored", "staging", nil) require.NoError(t, err) require.Len(t, result, 1) @@ -178,7 +178,7 @@ func TestGroupFilesByNamespace_EnvSubstitution_FlatLayout(t *testing.T) { "stations.sql": "CREATE TABLE stations (...);", } - result, err := GroupFilesByNamespace(files, "bikeshare_$ENV", "production") + result, _, err := GroupFilesByNamespace(files, "bikeshare_$ENV", "production", nil) require.NoError(t, err) require.Len(t, result, 1) @@ -192,7 +192,7 @@ func TestGroupFilesByNamespace_EnvSubstitution_EmptyEnvNoChange(t *testing.T) { "bikeshare_$ENV/bikes.sql": "CREATE TABLE bikes (...);", } - result, err := GroupFilesByNamespace(files, "ignored", "") + result, _, err := GroupFilesByNamespace(files, "ignored", "", nil) require.NoError(t, err) require.Len(t, result, 1) @@ -206,10 +206,132 @@ func TestGroupFilesByNamespace_EnvSubstitution_MultipleNamespaces(t *testing.T) "analytics/events.sql": "CREATE TABLE events (...);", } - result, err := GroupFilesByNamespace(files, "ignored", "staging") + result, _, err := GroupFilesByNamespace(files, "ignored", "staging", nil) require.NoError(t, err) require.Len(t, result, 2) assert.Contains(t, result, "app_staging") assert.Contains(t, result, "analytics") } + +func TestGroupFilesByNamespace_IgnoreNamespaces(t *testing.T) { + // A namespace directory listed in ignore_namespaces is excluded from the + // result β€” its files never reach plans, applies, or checks β€” while the + // other namespaces are grouped normally. + files := map[string]string{ + "payments/users.sql": "CREATE TABLE users (...);", + "payments/vschema.json": `{"tables": {}}`, + "local_fixtures/widgets.sql": "CREATE TABLE widgets (...);", + } + + result, removed, err := GroupFilesByNamespace(files, "ignored", "development", []string{"local_fixtures"}) + require.NoError(t, err) + + require.Len(t, result, 1) + require.Contains(t, result, "payments") + assert.Contains(t, result["payments"].Files, "users.sql") + assert.Contains(t, result["payments"].Files, "vschema.json") + assert.NotContains(t, result, "local_fixtures") + assert.Equal(t, []string{"local_fixtures"}, removed) +} + +func TestGroupFilesByNamespace_IgnoreNamespacesEnvSubstitution(t *testing.T) { + // Ignore entries receive the same $ENV substitution as directory names, + // so "fixtures_$ENV" excludes the "fixtures_staging" namespace when + // planning for staging. + files := map[string]string{ + "app_$ENV/users.sql": "CREATE TABLE users (...);", + "fixtures_$ENV/widgets.sql": "CREATE TABLE widgets (...);", + } + + result, removed, err := GroupFilesByNamespace(files, "ignored", "staging", []string{"fixtures_$ENV"}) + require.NoError(t, err) + + require.Len(t, result, 1) + assert.Contains(t, result, "app_staging") + assert.Equal(t, []string{"fixtures_staging"}, removed) +} + +func TestGroupFilesByNamespace_IgnoreNamespacesNoMatch(t *testing.T) { + // An ignore entry that matches no namespace directory removes nothing; the + // removed list stays empty and the entry surfaces via + // UnmatchedIgnoreEntries so callers can warn instead of implying an + // exclusion that is not happening. + files := map[string]string{ + "payments/users.sql": "CREATE TABLE users (...);", + } + + result, removed, err := GroupFilesByNamespace(files, "ignored", "development", []string{"nonexistent"}) + require.NoError(t, err) + + require.Len(t, result, 1) + assert.Contains(t, result, "payments") + assert.Empty(t, removed) + assert.Equal(t, []string{"nonexistent"}, UnmatchedIgnoreEntries([]string{"nonexistent"}, "development", removed)) +} + +func TestGroupFilesByNamespace_IgnoreNamespacesCaseSensitive(t *testing.T) { + // Matching is exact and case-sensitive: an entry that differs from the + // directory name only by case removes nothing, and is reported unmatched + // so the mismatch is visible rather than silently reconciling the + // namespace it was meant to exclude. + files := map[string]string{ + "payments/users.sql": "CREATE TABLE users (...);", + } + + result, removed, err := GroupFilesByNamespace(files, "ignored", "development", []string{"Payments"}) + require.NoError(t, err) + + require.Len(t, result, 1) + assert.Contains(t, result, "payments") + assert.Empty(t, removed) + assert.Equal(t, []string{"Payments"}, UnmatchedIgnoreEntries([]string{"Payments"}, "development", removed)) +} + +func TestUnmatchedIgnoreEntries(t *testing.T) { + assert.Nil(t, UnmatchedIgnoreEntries(nil, "staging", nil)) + + // Entries are resolved with $ENV before comparison against the removed + // keys, so a matched $ENV entry is not reported. + assert.Nil(t, UnmatchedIgnoreEntries( + []string{"fixtures_$ENV"}, "staging", []string{"fixtures_staging"})) + assert.Equal(t, []string{"typo"}, UnmatchedIgnoreEntries( + []string{"fixtures_$ENV", "typo"}, "staging", []string{"fixtures_staging"})) +} + +func TestValidateIgnoreNamespaces(t *testing.T) { + assert.NoError(t, ValidateIgnoreNamespaces(nil)) + assert.NoError(t, ValidateIgnoreNamespaces([]string{"local_fixtures", "fixtures_$ENV"})) + + err := ValidateIgnoreNamespaces([]string{""}) + require.Error(t, err) + assert.Contains(t, err.Error(), "must not be blank") + + err = ValidateIgnoreNamespaces([]string{" "}) + require.Error(t, err) + assert.Contains(t, err.Error(), "must not be blank") + + err = ValidateIgnoreNamespaces([]string{"local_fixtures "}) + require.Error(t, err) + assert.Contains(t, err.Error(), "whitespace") + + err = ValidateIgnoreNamespaces([]string{" local_fixtures"}) + require.Error(t, err) + assert.Contains(t, err.Error(), "whitespace") + + err = ValidateIgnoreNamespaces([]string{"schema/local_fixtures"}) + require.Error(t, err) + assert.Contains(t, err.Error(), "not a path") + + err = ValidateIgnoreNamespaces([]string{`schema\local_fixtures`}) + require.Error(t, err) + assert.Contains(t, err.Error(), "not a path") +} + +func TestResolveIgnoreNamespaces(t *testing.T) { + assert.Nil(t, ResolveIgnoreNamespaces(nil, "staging")) + assert.Equal(t, []string{"fixtures_staging", "local_fixtures"}, + ResolveIgnoreNamespaces([]string{"fixtures_$ENV", "local_fixtures"}, "staging")) + assert.Equal(t, []string{"fixtures_$ENV"}, + ResolveIgnoreNamespaces([]string{"fixtures_$ENV"}, "")) +} diff --git a/pkg/tern/local_client.go b/pkg/tern/local_client.go index 8846609c0..10a1b72fe 100644 --- a/pkg/tern/local_client.go +++ b/pkg/tern/local_client.go @@ -1623,6 +1623,19 @@ func (c *LocalClient) planWithEngine(ctx context.Context, req *ternv1.PlanReques return nil, err } if hasDatabase { + // A database-scoped target DSN diffs the whole database as one unit: + // the engine loads every live table while the desired state is only + // the files the caller sent. A namespace withheld via + // ignore_namespaces leaves its live tables with no declaring file, so + // the declarative diff would plan them as drops β€” the inverse of + // "ignore". No reliable live-tableβ†’namespace mapping exists on this + // shape, so refuse rather than emit a plan that proposes dropping the + // namespace the config says to leave alone. + if len(req.GetIgnoredNamespaces()) > 0 { + return nil, fmt.Errorf( + "ignore_namespaces is not supported for MySQL targets whose DSN names a database: the whole database is diffed as one unit, so ignored namespaces %v would have their live tables planned as DROP TABLE; use a namespace-free target DSN or remove ignore_namespaces", + req.GetIgnoredNamespaces()) + } return c.planNamespaceWithEngine(ctx, eng, req, database, schemaFiles, c.credentials()) } if len(schemaFiles) == 0 { diff --git a/pkg/tern/local_client_test.go b/pkg/tern/local_client_test.go index da6638aba..a9c759b29 100644 --- a/pkg/tern/local_client_test.go +++ b/pkg/tern/local_client_test.go @@ -19,6 +19,7 @@ import ( "github.com/block/schemabot/pkg/engine" ternv1 "github.com/block/schemabot/pkg/proto/ternv1" "github.com/block/schemabot/pkg/psclient" + "github.com/block/schemabot/pkg/schema" "github.com/block/schemabot/pkg/state" "github.com/block/schemabot/pkg/storage" ) @@ -750,6 +751,28 @@ func TestLocalClient_Apply_RequiresEnvironmentField(t *testing.T) { require.ErrorContains(t, err, "environment is required") } +// A database-scoped MySQL target DSN diffs the whole database as one unit, so +// a namespace withheld via ignore_namespaces would leave its live tables with +// no declaring file and the diff would plan them as DROP TABLE β€” the inverse +// of "ignore". The plan must refuse this combination up front rather than +// emit a destructive plan. +func TestPlanWithEngine_RefusesIgnoredNamespacesOnDatabaseScopedMySQLDSN(t *testing.T) { + client, err := NewLocalClient(LocalConfig{ + Database: "testdb", + Type: storage.DatabaseTypeMySQL, + TargetDSN: "user:pass@tcp(localhost:3306)/testdb", + }, nil, slog.Default()) + require.NoError(t, err) + + _, err = client.planWithEngine(t.Context(), &ternv1.PlanRequest{ + Database: "testdb", + IgnoredNamespaces: []string{"local_fixtures"}, + }, "testdb", schema.SchemaFiles{"testdb": {Files: map[string]string{"users.sql": "CREATE TABLE users (id INT)"}}}) + require.Error(t, err) + assert.Contains(t, err.Error(), "ignore_namespaces is not supported for MySQL targets whose DSN names a database") + assert.Contains(t, err.Error(), "local_fixtures") +} + func TestRejectUnsafeDDLChangesWithoutOptIn(t *testing.T) { changes := []storage.TableChange{{ Namespace: "testdb", diff --git a/pkg/webhook/apply_execute.go b/pkg/webhook/apply_execute.go index d6c2108e8..5cc975908 100644 --- a/pkg/webhook/apply_execute.go +++ b/pkg/webhook/apply_execute.go @@ -32,15 +32,16 @@ func (h *Handler) executeApply( // Re-plan for drift detection prNumber := int32(pr) planReq := api.PlanRequest{ - Database: schemaResult.Database, - Environment: environment, - Type: schemaResult.Type, - SchemaFiles: schemaResult.SchemaFiles, - Repository: repo, - PullRequest: &prNumber, - HeadSHA: &schemaResult.HeadSHA, - SchemaPath: schemaResult.SchemaPath, - SourceTrusted: true, + Database: schemaResult.Database, + Environment: environment, + Type: schemaResult.Type, + SchemaFiles: schemaResult.SchemaFiles, + Repository: repo, + PullRequest: &prNumber, + HeadSHA: &schemaResult.HeadSHA, + SchemaPath: schemaResult.SchemaPath, + IgnoredNamespaces: schemaResult.IgnoredNamespaces, + SourceTrusted: true, } planResp, err := h.executePlanWithTransientRetry(ctx, planReq, repo, pr) diff --git a/pkg/webhook/apply_handlers.go b/pkg/webhook/apply_handlers.go index 23a254989..294e03324 100644 --- a/pkg/webhook/apply_handlers.go +++ b/pkg/webhook/apply_handlers.go @@ -243,15 +243,16 @@ func (h *Handler) applyCommandCore(parent context.Context, repo string, pr int, // Generate plan prNumber := int32(pr) planReq := api.PlanRequest{ - Database: schemaResult.Database, - Environment: environment, - Type: schemaResult.Type, - SchemaFiles: schemaResult.SchemaFiles, - Repository: repo, - PullRequest: &prNumber, - HeadSHA: &schemaResult.HeadSHA, - SchemaPath: schemaResult.SchemaPath, - SourceTrusted: true, + Database: schemaResult.Database, + Environment: environment, + Type: schemaResult.Type, + SchemaFiles: schemaResult.SchemaFiles, + Repository: repo, + PullRequest: &prNumber, + HeadSHA: &schemaResult.HeadSHA, + SchemaPath: schemaResult.SchemaPath, + IgnoredNamespaces: schemaResult.IgnoredNamespaces, + SourceTrusted: true, } planResp, err := h.executePlanWithTransientRetry(ctx, planReq, repo, pr) diff --git a/pkg/webhook/plan.go b/pkg/webhook/plan.go index d81fb4801..d745bf2bc 100644 --- a/pkg/webhook/plan.go +++ b/pkg/webhook/plan.go @@ -106,15 +106,16 @@ func (h *Handler) handlePlanCommand(w http.ResponseWriter, repo string, pr int, deployment = resolvedTarget.Deployment } planReq := api.PlanRequest{ - Database: schemaResult.Database, - Environment: environment, - Type: schemaResult.Type, - SchemaFiles: schemaResult.SchemaFiles, - Repository: repo, - PullRequest: &prNumber, - HeadSHA: &schemaResult.HeadSHA, - SchemaPath: schemaResult.SchemaPath, - SourceTrusted: true, + Database: schemaResult.Database, + Environment: environment, + Type: schemaResult.Type, + SchemaFiles: schemaResult.SchemaFiles, + Repository: repo, + PullRequest: &prNumber, + HeadSHA: &schemaResult.HeadSHA, + SchemaPath: schemaResult.SchemaPath, + IgnoredNamespaces: schemaResult.IgnoredNamespaces, + SourceTrusted: true, } // Execute plan via the service @@ -390,15 +391,16 @@ func (h *Handler) handleMultiEnvPlan(repo string, pr int, databaseName, tenant s prNumber := int32(pr) planReq := api.PlanRequest{ - Database: schemaResult.Database, - Environment: env, - Type: schemaResult.Type, - SchemaFiles: schemaResult.SchemaFiles, - Repository: repo, - PullRequest: &prNumber, - HeadSHA: &schemaResult.HeadSHA, - SchemaPath: schemaResult.SchemaPath, - SourceTrusted: true, + Database: schemaResult.Database, + Environment: env, + Type: schemaResult.Type, + SchemaFiles: schemaResult.SchemaFiles, + Repository: repo, + PullRequest: &prNumber, + HeadSHA: &schemaResult.HeadSHA, + SchemaPath: schemaResult.SchemaPath, + IgnoredNamespaces: schemaResult.IgnoredNamespaces, + SourceTrusted: true, } planProto, planResp, err := h.executePlanProtoWithTransientRetry(ctx, planReq, repo, pr) @@ -748,15 +750,16 @@ func shardedBlockedChanges(shards []*apitypes.ShardPlanResponse) []templates.Blo // buildPlanCommentData converts plan results into template data. func buildPlanCommentData(schema *ghclient.SchemaRequestResult, planResp *apitypes.PlanResponse, environment, tenant, requestedBy, agentHint string) templates.PlanCommentData { data := templates.PlanCommentData{ - Database: schema.Database, - Environment: environment, - Tenant: tenant, - AgentHint: agentHint, - HeadSHA: schema.HeadSHA, - Repository: schema.Repository, - RequestedBy: requestedBy, - DatabaseType: schema.Type, - IsMySQL: schema.Type == "mysql", + Database: schema.Database, + Environment: environment, + Tenant: tenant, + AgentHint: agentHint, + HeadSHA: schema.HeadSHA, + Repository: schema.Repository, + RequestedBy: requestedBy, + DatabaseType: schema.Type, + IsMySQL: schema.Type == "mysql", + IgnoredNamespaces: schema.IgnoredNamespaces, } // Per-shard changes, grouped by keyspace, so a sharded keyspace can show what diff --git a/pkg/webhook/plan_integration_test.go b/pkg/webhook/plan_integration_test.go index b0bbe4282..64546cf6b 100644 --- a/pkg/webhook/plan_integration_test.go +++ b/pkg/webhook/plan_integration_test.go @@ -905,3 +905,134 @@ func TestE2EPlanUsesServerSideTarget(t *testing.T) { } // --- Container helpers (matches e2e/setup_test.go patterns) --- + +// TestE2EPlanExcludesIgnoredNamespaces verifies that namespace directories +// listed in schemabot.yaml ignore_namespaces never reach the plan: a schema +// root carrying a local-test-only namespace alongside the real one produces a +// plan (comment, stored record, check) covering only the real namespace, so +// SchemaBot never tries to reconcile test fixtures against the live database. +func TestE2EPlanExcludesIgnoredNamespaces(t *testing.T) { + dbName := "webhook_plan_ignore_ns" + // ignore_namespaces requires the namespace-free MySQL DSN shape (a + // database-scoped DSN diffs the whole database as one unit and refuses the + // exclusion), so this scenario configures the target the supported way. + svc := setupE2EServiceOpts(t, dbName, e2eServiceOpts{targetDSN: namespaceFreeTargetDSN(t)}) + dbConfig := svc.Config().Databases[dbName] + dbConfig.AllowedRepos = []string{"octocat/hello-world"} + dbConfig.AllowedDirs = []string{"schema"} + svc.Config().Databases[dbName] = dbConfig + + mux := http.NewServeMux() + server := httptest.NewServer(mux) + t.Cleanup(server.Close) + + client := gh.NewClient(nil) + client.BaseURL, _ = url.Parse(server.URL + "/") + + schemabotConfig := fmt.Sprintf("database: %s\ntype: mysql\nignore_namespaces:\n - local_fixtures\n", dbName) + schemaFiles := map[string]string{ + dbName + "/users.sql": "CREATE TABLE `users` (\n `id` bigint unsigned NOT NULL AUTO_INCREMENT,\n `name` varchar(255) NOT NULL,\n PRIMARY KEY (`id`)\n) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_ai_ci;", + "local_fixtures/widgets.sql": "CREATE TABLE `widgets` (\n `id` bigint unsigned NOT NULL AUTO_INCREMENT,\n PRIMARY KEY (`id`)\n) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_ai_ci;", + } + + result := setupFakeGitHubForPlan(t, mux, schemaFiles, schemabotConfig, dbName) + + logger := slog.New(slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{Level: slog.LevelError})) + installClient := ghclient.NewInstallationClient(client, logger) + factory := &fakeClientFactory{client: installClient} + + h := NewHandler(svc, factory, nil, logger) + + req := buildWebhookRequest(t, webhookPayloadOpts{ + comment: "schemabot plan -e staging", + isPR: true, + }, nil) + + rr := httptest.NewRecorder() + h.ServeHTTP(rr, req) + + require.Equal(t, http.StatusOK, rr.Code) + assert.Contains(t, rr.Body.String(), "plan generated successfully") + + // The plan comment covers the real namespace only, and discloses the + // exclusion so a reviewer can tell the namespace was withheld by config + // rather than unchanged. + select { + case body := <-result.comments: + assert.Contains(t, body, "## Schema Change Plan") + assert.Contains(t, body, "users") + assert.NotContains(t, body, "widgets") + assert.Contains(t, body, "ℹ️ Namespaces excluded from this plan by `ignore_namespaces`: `local_fixtures`") + case <-time.After(10 * time.Second): + t.Fatal("timed out waiting for plan comment") + } + + // The stored plan record carries no trace of the ignored namespace. + ctx := t.Context() + plans, err := svc.Storage().Plans().GetByPR(ctx, "octocat/hello-world", 1) + require.NoError(t, err) + var plan *storage.Plan + for _, p := range plans { + if p.Database == dbName { + plan = p + break + } + } + require.NotNil(t, plan, "expected a plan record for database %s", dbName) + require.NotNil(t, plan.Namespaces) + assert.Contains(t, plan.Namespaces, dbName) + assert.NotContains(t, plan.Namespaces, "local_fixtures") +} + +// TestE2EPlanIgnoringEveryNamespaceFails verifies the safety boundary when +// ignore_namespaces excludes every namespace in the schema root: the plan +// must fail with an error naming the exclusion rather than planning an empty +// desired state, which on a live database would read as "drop everything". +func TestE2EPlanIgnoringEveryNamespaceFails(t *testing.T) { + dbName := "webhook_plan_ignore_all" + svc := setupE2EService(t, dbName) + dbConfig := svc.Config().Databases[dbName] + dbConfig.AllowedRepos = []string{"octocat/hello-world"} + dbConfig.AllowedDirs = []string{"schema"} + svc.Config().Databases[dbName] = dbConfig + + mux := http.NewServeMux() + server := httptest.NewServer(mux) + t.Cleanup(server.Close) + + client := gh.NewClient(nil) + client.BaseURL, _ = url.Parse(server.URL + "/") + + schemabotConfig := fmt.Sprintf("database: %s\ntype: mysql\nignore_namespaces:\n - %s\n - local_fixtures\n", dbName, dbName) + schemaFiles := map[string]string{ + dbName + "/users.sql": "CREATE TABLE `users` (\n `id` bigint unsigned NOT NULL AUTO_INCREMENT,\n PRIMARY KEY (`id`)\n) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_ai_ci;", + "local_fixtures/widgets.sql": "CREATE TABLE `widgets` (\n `id` bigint unsigned NOT NULL AUTO_INCREMENT,\n PRIMARY KEY (`id`)\n) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_ai_ci;", + } + + result := setupFakeGitHubForPlan(t, mux, schemaFiles, schemabotConfig, dbName) + + logger := slog.New(slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{Level: slog.LevelError})) + installClient := ghclient.NewInstallationClient(client, logger) + factory := &fakeClientFactory{client: installClient} + + h := NewHandler(svc, factory, nil, logger) + + req := buildWebhookRequest(t, webhookPayloadOpts{ + comment: "schemabot plan -e staging", + isPR: true, + }, nil) + + rr := httptest.NewRecorder() + h.ServeHTTP(rr, req) + + require.Equal(t, http.StatusOK, rr.Code) + assert.Contains(t, rr.Body.String(), "schema request error handled") + + select { + case body := <-result.comments: + assert.Contains(t, body, "after excluding ignored namespaces") + assert.NotContains(t, body, "No schema changes detected") + case <-time.After(10 * time.Second): + t.Fatal("timed out waiting for error comment") + } +} diff --git a/pkg/webhook/templates/ignored_namespaces_test.go b/pkg/webhook/templates/ignored_namespaces_test.go new file mode 100644 index 000000000..56b1f570b --- /dev/null +++ b/pkg/webhook/templates/ignored_namespaces_test.go @@ -0,0 +1,140 @@ +package templates + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" +) + +// A plan whose repository config withheld namespaces via ignore_namespaces +// discloses the exclusion on the comment, directly under the plan summary, so +// a reviewer reads what was counted and then what was deliberately not +// reconciled. +func TestRenderPlanComment_IgnoredNamespacesDisclosed(t *testing.T) { + out := RenderPlanComment(PlanCommentData{ + Database: "testapp", Environment: "staging", IsMySQL: true, + IgnoredNamespaces: []string{"fixtures_staging", "local_fixtures"}, + Changes: []KeyspaceChangeData{{ + Keyspace: "testapp", + Statements: []string{"ALTER TABLE `users` ADD COLUMN `email` varchar(255)"}, + }}, + }) + + disclosure := "ℹ️ Namespaces excluded from this plan by `ignore_namespaces`: `fixtures_staging`, `local_fixtures`" + assert.Contains(t, out, disclosure) + summaryAt := strings.Index(out, "πŸ“‹ **Plan**:") + disclosureAt := strings.Index(out, disclosure) + assert.Greater(t, disclosureAt, summaryAt, "disclosure should render under the plan summary") +} + +// The disclosure renders on a no-changes plan too: without it, "no schema +// changes detected" is indistinguishable from "a namespace's changes were +// withheld by config" β€” the exact ambiguity the disclosure exists to remove. +func TestRenderPlanComment_IgnoredNamespacesDisclosedOnNoChanges(t *testing.T) { + out := RenderPlanComment(PlanCommentData{ + Database: "testapp", Environment: "staging", IsMySQL: true, + IgnoredNamespaces: []string{"local_fixtures"}, + }) + + disclosure := "ℹ️ Namespaces excluded from this plan by `ignore_namespaces`: `local_fixtures`" + assert.Contains(t, out, disclosure) + assert.Contains(t, out, "βœ… **No schema changes detected**") + assert.Greater(t, strings.Index(out, disclosure), strings.Index(out, "βœ… **No schema changes detected**"), + "disclosure should render under the no-changes message") +} + +// Plans from repositories without ignore_namespaces render unchanged. +func TestRenderPlanComment_NoIgnoredNamespacesNoDisclosure(t *testing.T) { + out := RenderPlanComment(PlanCommentData{ + Database: "testapp", Environment: "staging", IsMySQL: true, + Changes: []KeyspaceChangeData{{ + Keyspace: "testapp", + Statements: []string{"ALTER TABLE `users` ADD COLUMN `email` varchar(255)"}, + }}, + }) + + assert.NotContains(t, out, "ignore_namespaces") +} + +// ignore_namespaces entries can resolve differently per environment ($ENV +// substitution), so environments whose exclusions differ must not deduplicate +// into one shared section β€” that would show one environment's disclosure for +// both. Each environment's section carries its own list. +func TestRenderMultiEnvPlanComment_IgnoredNamespacesBlockDedup(t *testing.T) { + changes := []KeyspaceChangeData{{ + Keyspace: "testapp", + Statements: []string{"ALTER TABLE `users` ADD COLUMN `email` varchar(255)"}, + }} + out := RenderMultiEnvPlanComment(MultiEnvPlanCommentData{ + Database: "testapp", DatabaseType: "mysql", IsMySQL: true, + Environments: []string{"staging", "production"}, + Plans: map[string]*PlanCommentData{ + "staging": {Environment: "staging", IsMySQL: true, Changes: changes, IgnoredNamespaces: []string{"fixtures_staging"}}, + "production": {Environment: "production", IsMySQL: true, Changes: changes, IgnoredNamespaces: []string{"fixtures_production"}}, + }, + }) + + assert.Contains(t, out, "### Staging") + assert.Contains(t, out, "### Production") + assert.Contains(t, out, "ℹ️ Namespaces excluded from this plan by `ignore_namespaces`: `fixtures_staging`") + assert.Contains(t, out, "ℹ️ Namespaces excluded from this plan by `ignore_namespaces`: `fixtures_production`") +} + +// Environments with identical DDL and identical exclusions still deduplicate +// into one combined section, disclosing the shared exclusion once. +func TestRenderMultiEnvPlanComment_IgnoredNamespacesIdenticalDedup(t *testing.T) { + changes := []KeyspaceChangeData{{ + Keyspace: "testapp", + Statements: []string{"ALTER TABLE `users` ADD COLUMN `email` varchar(255)"}, + }} + out := RenderMultiEnvPlanComment(MultiEnvPlanCommentData{ + Database: "testapp", DatabaseType: "mysql", IsMySQL: true, + Environments: []string{"staging", "production"}, + Plans: map[string]*PlanCommentData{ + "staging": {Environment: "staging", IsMySQL: true, Changes: changes, IgnoredNamespaces: []string{"local_fixtures"}}, + "production": {Environment: "production", IsMySQL: true, Changes: changes, IgnoredNamespaces: []string{"local_fixtures"}}, + }, + }) + + assert.Contains(t, out, "### Staging & Production") + assert.Equal(t, 1, strings.Count(out, "ℹ️ Namespaces excluded from this plan by `ignore_namespaces`: `local_fixtures`")) +} + +// The all-environments-clean short circuit has no per-environment sections, so +// the disclosure renders alongside the combined no-changes message β€” an +// all-clean result is exactly where a reviewer needs to see that a namespace +// was withheld rather than genuinely unchanged. +func TestRenderMultiEnvPlanComment_IgnoredNamespacesOnAllClean(t *testing.T) { + out := RenderMultiEnvPlanComment(MultiEnvPlanCommentData{ + Database: "testapp", DatabaseType: "mysql", IsMySQL: true, + Environments: []string{"staging", "production"}, + Plans: map[string]*PlanCommentData{ + "staging": {Environment: "staging", IsMySQL: true, IgnoredNamespaces: []string{"local_fixtures"}}, + "production": {Environment: "production", IsMySQL: true, IgnoredNamespaces: []string{"local_fixtures"}}, + }, + }) + + assert.Contains(t, out, "βœ… **No schema changes detected** for any environment.") + disclosure := "ℹ️ Namespaces excluded from this plan by `ignore_namespaces`: `local_fixtures`" + assert.Equal(t, 1, strings.Count(out, disclosure)) + assert.Greater(t, strings.Index(out, disclosure), strings.Index(out, "βœ… **No schema changes detected** for any environment."), + "disclosure should render under the no-changes message") +} + +// When the all-clean environments excluded different namespaces, the combined +// message discloses each environment's own list, labeled by environment. +func TestRenderMultiEnvPlanComment_IgnoredNamespacesOnAllCleanPerEnv(t *testing.T) { + out := RenderMultiEnvPlanComment(MultiEnvPlanCommentData{ + Database: "testapp", DatabaseType: "mysql", IsMySQL: true, + Environments: []string{"staging", "production"}, + Plans: map[string]*PlanCommentData{ + "staging": {Environment: "staging", IsMySQL: true, IgnoredNamespaces: []string{"fixtures_staging"}}, + "production": {Environment: "production", IsMySQL: true, IgnoredNamespaces: []string{"fixtures_production"}}, + }, + }) + + assert.Contains(t, out, "βœ… **No schema changes detected** for any environment.") + assert.Contains(t, out, "ℹ️ **Staging**: namespaces excluded from this plan by `ignore_namespaces`: `fixtures_staging`") + assert.Contains(t, out, "ℹ️ **Production**: namespaces excluded from this plan by `ignore_namespaces`: `fixtures_production`") +} diff --git a/pkg/webhook/templates/plan.go b/pkg/webhook/templates/plan.go index 79bdd447b..9501abf0e 100644 --- a/pkg/webhook/templates/plan.go +++ b/pkg/webhook/templates/plan.go @@ -3,6 +3,7 @@ package templates import ( "fmt" "html" + "slices" "strings" "github.com/block/schemabot/pkg/caller" @@ -87,6 +88,14 @@ type PlanCommentData struct { LintViolations []LintViolationData Errors []string + // IgnoredNamespaces lists the namespaces whose schema files were excluded + // from this plan by the repository's ignore_namespaces config β€” only entries + // that actually removed a namespace, resolved and sorted. Disclosed on the + // comment so a reviewer can tell "this namespace has no changes" apart from + // "this namespace was withheld by config", which is what makes a PR that + // introduces an ignore_namespaces entry visible in review. + IgnoredNamespaces []string + // Unsafe change tracking HasUnsafeChanges bool AllowUnsafe bool @@ -170,9 +179,16 @@ func RenderPlanComment(data PlanCommentData) string { totalStatements, keyspacesWithVSchema := countChanges(data.Changes) totalChanges := totalStatements + keyspacesWithVSchema - // No changes β€” short-circuit with a single clean message + // No changes β€” short-circuit with a single clean message. The + // ignore_namespaces disclosure still renders: a no-changes result is + // exactly where a reviewer needs to tell a withheld namespace apart from a + // genuinely unchanged one. if totalChanges == 0 { writeNoChangesDetected(&sb, data) + if len(data.IgnoredNamespaces) > 0 { + sb.WriteString("\n") + writeIgnoredNamespaces(&sb, data.IgnoredNamespaces) + } return appendAgentHint(sb.String(), data.AgentHint) } @@ -390,6 +406,7 @@ func writePlanSummary(sb *strings.Builder, data PlanCommentData, totalStatements if totalChanges == 0 { writeNoChangesDetected(sb, data) sb.WriteString("\n") + writeIgnoredNamespaces(sb, data.IgnoredNamespaces) return } @@ -416,6 +433,79 @@ func writePlanSummary(sb *strings.Builder, data PlanCommentData, totalStatements // Fallback for unrecognized statement types fmt.Fprintf(sb, "πŸ“‹ **Plan**: %d DDL %s\n\n", totalStatements, pluralize("statement", totalStatements)) } + + // Disclosed directly under the plan summary so the exclusion reads as + // part of the plan result: what was counted, then what was withheld. + writeIgnoredNamespaces(sb, data.IgnoredNamespaces) +} + +// writeIgnoredNamespaces renders the ignore_namespaces disclosure line. No-op +// when nothing was excluded, so plans from repos without the config render +// unchanged. +func writeIgnoredNamespaces(sb *strings.Builder, ignored []string) { + if len(ignored) == 0 { + return + } + quoted := make([]string, len(ignored)) + for i, ns := range ignored { + quoted[i] = fmt.Sprintf("`%s`", ns) + } + fmt.Fprintf(sb, "ℹ️ Namespaces excluded from this plan by `ignore_namespaces`: %s\n\n", strings.Join(quoted, ", ")) +} + +// multiEnvHasIgnoredNamespaces reports whether any environment's plan excluded +// namespaces, so callers can decide whether the disclosure (and its spacing) +// renders at all. +func multiEnvHasIgnoredNamespaces(data MultiEnvPlanCommentData) bool { + for _, env := range data.Environments { + if plan, ok := data.Plans[env]; ok && plan != nil && len(plan.IgnoredNamespaces) > 0 { + return true + } + } + return false +} + +// writeMultiEnvIgnoredNamespaces renders the ignore_namespaces disclosure for +// the all-environments-clean path, where no per-environment sections exist to +// carry it. When every environment excluded the same namespaces it renders the +// single shared line; otherwise one line per environment, since entries can +// resolve differently per environment. +func writeMultiEnvIgnoredNamespaces(sb *strings.Builder, data MultiEnvPlanCommentData) { + anyIgnored := false + identical := true + var first []string + for i, env := range data.Environments { + var ignored []string + if plan, ok := data.Plans[env]; ok && plan != nil { + ignored = plan.IgnoredNamespaces + } + if len(ignored) > 0 { + anyIgnored = true + } + if i == 0 { + first = ignored + } else if !slices.Equal(ignored, first) { + identical = false + } + } + if !anyIgnored { + return + } + if identical { + writeIgnoredNamespaces(sb, first) + return + } + for _, env := range data.Environments { + plan, ok := data.Plans[env] + if !ok || plan == nil || len(plan.IgnoredNamespaces) == 0 { + continue + } + quoted := make([]string, len(plan.IgnoredNamespaces)) + for i, ns := range plan.IgnoredNamespaces { + quoted[i] = fmt.Sprintf("`%s`", ns) + } + fmt.Fprintf(sb, "ℹ️ **%s**: namespaces excluded from this plan by `ignore_namespaces`: %s\n\n", capitalizeFirst(env), strings.Join(quoted, ", ")) + } } func writeNoChangesDetected(sb *strings.Builder, data PlanCommentData) { @@ -934,9 +1024,16 @@ func RenderMultiEnvPlanComment(data MultiEnvPlanCommentData) string { } hasErrors := len(data.Errors) > 0 - // If no environments have changes and no errors, show simple message + // If no environments have changes and no errors, show simple message. The + // ignore_namespaces disclosure still renders underneath it: an all-clean + // result is exactly where a reviewer needs to see that a namespace was + // withheld rather than genuinely unchanged. if envsWithChanges == 0 && !hasErrors { sb.WriteString("βœ… **No schema changes detected** for any environment.\n") + if multiEnvHasIgnoredNamespaces(data) { + sb.WriteString("\n") + writeMultiEnvIgnoredNamespaces(&sb, data) + } return appendAgentHint(sb.String(), data.AgentHint) } @@ -1055,8 +1152,12 @@ func writeEnvironmentPlanSection(sb *strings.Builder, plan *PlanCommentData) { totalStatements, keyspacesWithVSchema := countChanges(plan.Changes) totalChanges := totalStatements + keyspacesWithVSchema + // The ignore_namespaces disclosure renders under each environment's + // summary (writePlanSummary) or no-changes message, because entries can + // resolve differently per environment. if totalChanges == 0 { sb.WriteString("βœ… **No schema changes detected**\n\n") + writeIgnoredNamespaces(sb, plan.IgnoredNamespaces) return } @@ -1194,8 +1295,13 @@ func allPlansIdentical(data MultiEnvPlanCommentData) bool { return firstPlan != nil } -// plansIdentical checks if two plans have the same DDL statements. +// plansIdentical checks if two plans have the same DDL statements. Plans that +// excluded different namespaces are never identical: deduplicating them into +// one section would show one environment's exclusion disclosure for both. func plansIdentical(a, b *PlanCommentData) bool { + if !slices.Equal(a.IgnoredNamespaces, b.IgnoredNamespaces) { + return false + } if len(a.Changes) != len(b.Changes) { return false } diff --git a/pkg/webhook/templates/preview.go b/pkg/webhook/templates/preview.go index 52647347c..686a750f6 100644 --- a/pkg/webhook/templates/preview.go +++ b/pkg/webhook/templates/preview.go @@ -58,6 +58,16 @@ func previewPlanData() PlanCommentData { } } +// PreviewCommentPlanIgnoredNamespaces renders a sample plan from a schema root +// whose config withholds a namespace via ignore_namespaces, showing the +// exclusion disclosure alongside the remaining namespace's changes. +func PreviewCommentPlanIgnoredNamespaces() string { + data := previewPlanData() + data.LintViolations = nil + data.IgnoredNamespaces = []string{"local_fixtures"} + return RenderPlanComment(data) +} + // PreviewCommentPlanBlocked renders a sample plan containing a statement the // engine deterministically refuses (execution-mode verdict "blocked"). func PreviewCommentPlanBlocked() string { diff --git a/pkg/webhook/webhook_integration_test.go b/pkg/webhook/webhook_integration_test.go index 83fcb17b5..8aecca8f5 100644 --- a/pkg/webhook/webhook_integration_test.go +++ b/pkg/webhook/webhook_integration_test.go @@ -180,10 +180,24 @@ type e2eServiceOpts struct { skipOperator bool // databaseType and targetDSN let integration scenarios exercise another // built-in engine while retaining the same API, storage, and operator path. + // For MySQL, a non-empty targetDSN overrides the default database-scoped + // DSN β€” e.g. a namespace-free DSN for scenarios where the namespace + // selects the database. databaseType string targetDSN string } +// namespaceFreeTargetDSN returns the shared MySQL target's DSN with no +// database selected β€” the shape where each namespace's directory name selects +// the database it is planned against. +func namespaceFreeTargetDSN(t *testing.T) string { + t.Helper() + cfg, err := mysql.ParseDSN(e2eTargetDSN) + require.NoError(t, err) + cfg.DBName = "" + return cfg.FormatDSN() +} + // setupE2EServiceOpts creates a real api.Service with a LocalClient for the // given database, customized by opts. func setupE2EServiceOpts(t *testing.T, appDBName string, opts e2eServiceOpts) *api.Service { @@ -211,10 +225,12 @@ func setupE2EServiceOpts(t *testing.T, appDBName string, opts e2eServiceOpts) *a } }) - cfg, err := mysql.ParseDSN(e2eTargetDSN) - require.NoError(t, err) - cfg.DBName = appDBName - appDSN = cfg.FormatDSN() + if appDSN == "" { + cfg, err := mysql.ParseDSN(e2eTargetDSN) + require.NoError(t, err) + cfg.DBName = appDBName + appDSN = cfg.FormatDSN() + } } require.NotEmpty(t, appDSN, "target DSN is required for database type %s", databaseType) logger := slog.New(slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{Level: slog.LevelError})) @@ -540,7 +556,9 @@ func registerCheckStatusRESTHandlersForAnyRef(mux *http.ServeMux, nodes func() [ } // setupFakeGitHubForPlan sets up a fake GitHub server for plan flows. -// schemaSQL maps filename -> content. Files are placed under schema/{namespace}/. +// schemaSQL maps filename -> content. Plain filenames are placed under +// schema/{namespace}/; filenames containing a "/" are placed under schema/ +// as-is, so a test can lay out multiple namespace subdirectories. // namespace is the MySQL schema name (required). func setupFakeGitHubForPlan(t *testing.T, mux *http.ServeMux, schemaSQL map[string]string, schemabotConfig, ns string) *planFlowResult { return setupFakeGitHubForPlanWithPRFiles(t, mux, schemaSQL, schemabotConfig, ns, nil) @@ -710,6 +728,16 @@ func treeLevelFrom(entries []*gh.TreeEntry, dir, subtreePrefix string) []*gh.Tre return level } +// schemaFixturePath resolves a schemaSQL key to its repository path: plain +// filenames live under the namespace subdirectory, keys with a "/" already +// name their namespace subdirectory. +func schemaFixturePath(ns, name string) string { + if strings.Contains(name, "/") { + return "schema/" + name + } + return "schema/" + ns + "/" + name +} + func setupFakeGitHubForPlanWithPRFiles(t *testing.T, mux *http.ServeMux, schemaSQL map[string]string, schemabotConfig, ns string, prFiles []*gh.CommitFile) *planFlowResult { t.Helper() @@ -775,7 +803,7 @@ func setupFakeGitHubForPlanWithPRFiles(t *testing.T, mux *http.ServeMux, schemaS var files []*gh.CommitFile for name := range schemaSQL { files = append(files, &gh.CommitFile{ - Filename: new("schema/" + ns + "/" + name), + Filename: new(schemaFixturePath(ns, name)), Status: new("added"), }) } @@ -805,7 +833,7 @@ func setupFakeGitHubForPlanWithPRFiles(t *testing.T, mux *http.ServeMux, schemaS blobIndex++ blobContents[sha] = content treeEntries = append(treeEntries, &gh.TreeEntry{ - Path: new("schema/" + ns + "/" + name), + Path: new(schemaFixturePath(ns, name)), Mode: new("100644"), Type: new("blob"), SHA: new(sha),