migrations: add missing index creating migration #1460
Conversation
crozzy
requested review from
RTann and
hdonnay
and removed request for
a team and
RTann
| case ms == nil, len(ms) == 1: | ||
| continue | ||
| case len(ms) == 2: | ||
| migrations = append(migrations, path.Clean(string(ms[1]))) |
There was a problem hiding this comment.
filepath? I guess it is always /, though
There was a problem hiding this comment.
I will change it but yeah
| sort.Strings(files) | ||
|
|
||
| // Check referenced files exist and existing files are referenced. | ||
| if !cmp.Equal(migrations, files) { |
There was a problem hiding this comment.
I believe this will fail on Windows machines due to the \? If so, is that ok?
There was a problem hiding this comment.
We don't support running on Windows so I think it's ok
| var migrations, files []string | ||
|
|
||
| // Get referenced migrations | ||
| migrationLine, err := regexp.Compile(`runFile\(\"(.*)\"\)`) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
While the sql command was added to claircore (and the admin command was added to clairctl) to create an index it was never actually reference in the migrations. This should be a no-op for all instances that ran the associated clairctl admin command. Also, added a test to try and avoid this in the future. Signed-off-by: crozzy <[email protected]>
crozzy
force-pushed
the
bug-add-migration
branch
from
31f43b0 to
e816c4f
Compare
While the sql command was added to claircore (and the admin command was added to clairctl) to create an index it was never actually reference in the migrations. This should be a no-op for all instances that ran the associated clairctl admin command. Also, added a test to try and avoid this in the future.