-
Notifications
You must be signed in to change notification settings - Fork 887
feat(coderd/database/dbtestutil): add ability to dump database on failure #9704
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.
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
Conversation
d06e9c1
to
1c4bf16
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
I would love for these to be uploaded as artifacts in CI, but if we're cautious about leaking secrets that might be an issue.
coderd/database/dbtestutil/db.go
Outdated
return | ||
} | ||
snakeCaseName := regexp.MustCompile("[^a-zA-Z0-9-_]+").ReplaceAllString(t.Name(), "_") | ||
outPath := filepath.Join(cwd, snakeCaseName+".test.sql") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about adding a random suffix to make sure we don't overwrite previous dumps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could add YYYYmmddHHMMSS
so you know at a glance which one is the most recent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: you can also consider adding a short guide on how to import the dump into a local Coder deployment
72dff7f included a provision to dump the contents of the database on failure, which actually seems really useful in a generic context.
dbtestutil
.t.Log
but this is also an option if running in CI. We just need to make sure to not leak any secrets.