Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

migrations: add missing index creating migration #1460

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

migrations: add missing index creating migration #1460

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions datastore/postgres/migrations/migrations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,10 @@ var IndexerMigrations = []migrate.Migration{
ID: 7,
Up: runFile("indexer/07-index-manifest_index.sql"),
},
{
ID: 8,
Up: runFile("indexer/08-index-manifest_layer.sql"),
},
}

var MatcherMigrations = []migrate.Migration{
Expand Down
68 changes: 68 additions & 0 deletions datastore/postgres/migrations/migrations_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
package migrations

import (
"bufio"
iofs "io/fs"
"os"
"path"
"path/filepath"
"regexp"
"slices"
"testing"

"github.com/google/go-cmp/cmp"
)

func TestMigrationsMismatch(t *testing.T) {
var migrations, files []string

// Get referenced migrations
migrationLine, err := regexp.Compile(`runFile\(\"(.*)\"\)`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels brittle. An idea can be to create a base slice like

const indexerMigrations = []string{
	"filename1",
	"filename2",
	...
}

and then have

func init() {
	for i, migration := range indexerMigrations {
		IndexerMigrations = append(IndexerMigrations, {
			ID: i+1,
			Up: runFile(migration),
		})
	}
}

something like that so we can just ensure indexerMigrations lists all the files

Copy link
Contributor Author

@crozzy crozzy Jan 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think one thing we'd want to keep is the ID being explicitly associated with the migration file so a user can check their DB migration version and know which was the last file being run (without having to work it out).

It is brittle in the sense that the runFile might change, but the proposed solution doesn't address fact that a sql file might exist on the FS but not be referenced in the go code, maybe this isn't actually an issue if you aren't forgetful (like myself) and remember to reference the migration. The reason this case was slightly different was that there was an admin command added to clairctl to pre-create the index so when testing the code in stage and production the index was already created and hence, it didn't look like anything was missing.

if err != nil {
t.Fatal(err)
}
f, err := os.Open("migrations.go")
if err != nil {
t.Fatal(err)
}
defer f.Close()

s := bufio.NewScanner(f)
for s.Scan() {
ms := migrationLine.FindSubmatch(s.Bytes())
switch {
case ms == nil, len(ms) == 1:
continue
case len(ms) == 2:
migrations = append(migrations, path.Clean(string(ms[1])))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

filepath? I guess it is always /, though

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will change it but yeah

}
}
if err := s.Err(); err != nil {
t.Error(err)
}
slices.Sort(migrations)

// Get migration files
err = iofs.WalkDir(os.DirFS("."), ".", func(p string, d iofs.DirEntry, err error) error {
switch {
case err != nil:
return err
case d.IsDir():
return nil
case filepath.Ext(d.Name()) != ".sql":
return nil
}
files = append(files, p)
return nil
})
if err != nil {
t.Error(err)
}
slices.Sort(files)

// Check referenced files exist and existing files are referenced.
if !cmp.Equal(migrations, files) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this will fail on Windows machines due to the \? If so, is that ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't support running on Windows so I think it's ok

t.Log("error mismatch of migrations to entries:")
t.Error(cmp.Diff(migrations, files))
}
}
Loading