feat(scripts): add script to check schema between migrations #13037

johnstcn · 2024-04-22T22:38:04Z

migrations: allow passing in a custom migrate.FS when calling migrate.Up
gen/dump: extract pg_dump used by gen/dump to dbtestutil, replace sed calls
scripts: write script to test migrations between schema versions

When run on the problematic commit with the gappy migrations:

go run ./scripts/migrate-test/main.go --from=bf296b2f1931aa259c573f43d58712f837139c1e --to=main
Read schema at version "main"
Read migrations for "bf296b2f1931aa259c573f43d58712f837139c1e"
Read migrations for "main"
Connect to postgres
No previous migration detected.
Init database at version "bf296b2f1931aa259c573f43d58712f837139c1e"
Migrate to version "main"
Dump schema at version "main"
Schema differs from expected after migration:   (
        """
        ... // 741 identical lines
        );
  
-       CREATE TABLE template_version_parameters (
-           template_version_id uuid NOT NULL,
-           name text NOT NULL,
-           description text NOT NULL,
-           type text NOT NULL,
-           mutable boolean NOT NULL,
-           default_value text NOT NULL,
-           icon text NOT NULL,
-           options jsonb DEFAULT '[]'::jsonb NOT NULL,
-           validation_regex text NOT NULL,
-           validation_min integer,
-           validation_max integer,
-           validation_error text DEFAULT ''::text NOT NULL,
-           validation_monotonic text DEFAULT ''::text NOT NULL,
-           required boolean DEFAULT true NOT NULL,
-           display_name text DEFAULT ''::text NOT NULL,
-           display_order integer DEFAULT 0 NOT NULL,
-           ephemeral boolean DEFAULT false NOT NULL,
-           CONSTRAINT validation_monotonic_order CHECK ((validation_monotonic = ANY (ARRAY['increasing'::text, 'decreasing'::text, ''::text])))
+       CREATE TABLE template_usage_stats (
+           start_time timestamp with time zone NOT NULL,
+           end_time timestamp with time zone NOT NULL,
+           template_id uuid NOT NULL,
+           user_id uuid NOT NULL,
+           median_latency_ms real,
+           usage_mins smallint NOT NULL,
+           ssh_mins smallint NOT NULL,
+           sftp_mins smallint NOT NULL,
+           reconnecting_pty_mins smallint NOT NULL,
+           vscode_mins smallint NOT NULL,
+           jetbrains_mins smallint NOT NULL,
+           app_usage_mins jsonb
        ... // 697 identical, 348 removed, and 408 inserted lines
        """
  )

exit status 1

When run on the subsequent commit that fixed it:

go run ./scripts/migrate-test/main.go --from=cfa5be52f7c57042690aa60fdbea8098ada93acc --to=main
Read schema at version "main"
Read migrations for "cfa5be52f7c57042690aa60fdbea8098ada93acc"
Read migrations for "main"
Connect to postgres
No previous migration detected.
Init database at version "cfa5be52f7c57042690aa60fdbea8098ada93acc"
Migrate to version "main"
Dump schema at version "main"
OK

- migrations: allow passing in a custom migrate.FS - gen/dump: extract some functions to dbtestutil - scripts: write script to test migrations

johnstcn · 2024-04-22T22:38:41Z

coderd/database/gen/dump/main.go

 	if err != nil {
 		panic(err)
 	}

-	for _, sed := range []string{


review: moved to dbtestutil.normalizeDump

johnstcn · 2024-04-22T22:38:52Z

coderd/database/gen/dump/main.go


 	err = migrations.Up(db)
 	if err != nil {
 		panic(err)
 	}

-	hasPGDump := false


review: moved to dbtestutil.PGDumpSchemaOnly

johnstcn · 2024-04-22T22:39:31Z

coderd/database/dbtestutil/db.go

+	schema = regexp.MustCompile(`(?im)^(--.*)$`).ReplaceAll(schema, []byte{})
+	// Public is implicit in the schema.
+	schema = regexp.MustCompile(`(?im)( |::|'|\()public\.`).ReplaceAll(schema, []byte(`$1`))
+	// Remove database settings.
+	schema = regexp.MustCompile(`(?im)^(SET.*;)`).ReplaceAll(schema, []byte(``))
+	// Remove select statements
+	schema = regexp.MustCompile(`(?im)^(SELECT.*;)`).ReplaceAll(schema, []byte(``))
+	// Removes multiple newlines.
+	schema = regexp.MustCompile(`(?im)\n{3,}`).ReplaceAll(schema, []byte("\n\n"))


review: we could pre-compile these but we've already lost enough readability by using regexen IMO

johnstcn · 2024-04-22T22:40:54Z

coderd/database/dbtestutil/db.go

-		_, _ = buf.WriteRune('\n')
+// PGDumpSchemaOnly is for use by gen/dump only.
+// It runs pg_dump against dbURL and sets a consistent timezone and encoding.
+func PGDumpSchemaOnly(dbURL string) ([]byte, error) {


review: I wanted to DRY this up with PGDump but it would have been more fiddly than I'd have liked.

johnstcn · 2024-04-22T22:41:14Z

coderd/database/dbtestutil/db.go

-func pgDump(dbURL string) ([]byte, error) {
+// PGDump runs pg_dump against dbURL and returns the output.
+// It is used by DumpOnFailure().
+func PGDump(dbURL string) ([]byte, error) {


review: Pre-emptively exported in case other packages find it useful.

johnstcn · 2024-04-22T22:41:38Z

coderd/database/migrations/migrate.go

+func Up(db *sql.DB) error {
+	return UpWithFS(db, migrations)
+}
+
+// UpWithFS runs SQL migrations in the given fs.
+func UpWithFS(db *sql.DB, migs fs.FS) (retErr error) {
+	_, m, err := setup(db, migs)


review: this make it possible to test arbitrary migration versions

scripts/migrate-test/main.go

mtojek · 2024-04-23T08:09:48Z

scripts/migrate-test/main.go

+
+func makeMigrateFS(version string) (fs.FS, error) {
+	// Export the migrations from the requested version to a zip archive
+	out, err := exec.Command("git", "archive", "--format=zip", version, "coderd/database/migrations").CombinedOutput()


if you want to go native, I bet you can find a library for git too.

Yep, there's https://github.com/go-git/go-git but I'm happy to just shell out here for now.

scripts/migrate-test/main.go

mtojek · 2024-04-23T08:15:47Z

scripts/migrate-test/main.go

+//  2. Checks out $NEW_VERSION and runs migrations.
+//  3. Compares database schema post-migrate to that in VCS.
+//     If any diffs are found, exits with an error.
+func main() {


Who/what/when is going to run this script? The hidden question is: can somebody forget to run it and there will be a disaster?

Point 👍 I need to add this to the CI workflow so it gets run on each commit.

Actually, it makes more sense to integrate with the script Mathias is working on. I'll add a Makefile target at least for now.

mtojek

This is in a good shape so feel free to merge it 👍

johnstcn · 2024-04-23T11:43:01Z

This is in a good shape so feel free to merge it 👍

Will add to a GH workflow in a separate PR

feat(scripts): add script to check schema between migrations

e117066

- migrations: allow passing in a custom migrate.FS - gen/dump: extract some functions to dbtestutil - scripts: write script to test migrations

github-actions bot assigned johnstcn Apr 22, 2024

johnstcn commented Apr 22, 2024

View reviewed changes

fixup comment

24ac2af

johnstcn requested review from mafredri and mtojek April 22, 2024 22:44

mtojek reviewed Apr 23, 2024

View reviewed changes

johnstcn added 2 commits April 23, 2024 10:42

add Makefile target

6e7fd5a

fixup! feat(scripts): add script to check schema between migrations

f8b9cb1

mtojek approved these changes Apr 23, 2024

View reviewed changes

johnstcn merged commit e57ca3c into main Apr 23, 2024
27 checks passed

johnstcn deleted the cj/test-migrations-upgrade branch April 23, 2024 11:43

github-actions bot locked and limited conversation to collaborators Apr 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(scripts): add script to check schema between migrations #13037

feat(scripts): add script to check schema between migrations #13037

johnstcn commented Apr 22, 2024

johnstcn Apr 22, 2024

johnstcn Apr 22, 2024

johnstcn Apr 22, 2024

johnstcn Apr 22, 2024

johnstcn Apr 22, 2024

johnstcn Apr 22, 2024

mtojek Apr 23, 2024

johnstcn Apr 23, 2024

mtojek Apr 23, 2024

johnstcn Apr 23, 2024

johnstcn Apr 23, 2024

mtojek left a comment

johnstcn commented Apr 23, 2024

feat(scripts): add script to check schema between migrations #13037

feat(scripts): add script to check schema between migrations #13037

Conversation

johnstcn commented Apr 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtojek left a comment

Choose a reason for hiding this comment

johnstcn commented Apr 23, 2024