internal/db: do not run TimescaleDB migrations against singleton database #17585
Conversation
|
Notifying subscribers in CODENOTIFY files for diff e45ae86...5fb7f14.
|
emidoots
added
code-insights
Codecov Report
@@ Coverage Diff @@
## main #17585 +/- ##
==========================================
- Coverage 51.53% 51.53% -0.01%
==========================================
Files 1717 1717
Lines 85401 85390 -11
Branches 7709 7709
==========================================
- Hits 44013 44003 -10
+ Misses 37511 37509 -2
- Partials 3877 3878 +1
*This pull request uses carry forward flags. Click here to find out more.
|
efritz
left a comment
efritz
left a comment
There was a problem hiding this comment.
Would it be a better solution to separate postgres from the code intel database rather than making exclusions to new things (that are probably more probably likely to act like code insights database than a code intelligence database)?
|
@efritz Im not sure I understand what you mean exactly, can you elaborate? |
|
Should we also separate Postgres and codeintel-db here? Instead of making timescale the odd one out, we can make the main "Postgres" db less of a "main" store and all three dbs would have equal tooling. |
|
Ahh, got it. I do think that is something we should do, I'll take a stab at that. |
…base In our DB testing code we make an assumption that we can run all database migrations against a single testing database. This holds true for frontend and codeintel migrations, because we intentionally designed codeintel migrations to be ran in the same DB for testing/dev purposes - but this does not hold true for Code Insights which runs a separate TimescaleDB instance (it is easier to run it separately in Docker than install it as a Postgres extension in all of our dev/testing environments.) This change merely makes it so we don't run TimescaleDB migrations against our singleton testing/dev Postgres DB. Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com>
emidoots
force-pushed
the
sg/insights-dbtesting
branch
from
1167cdb to
600a856
Compare
Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com>
|
@efritz PTAL, I think this is likely a better step in the right direction? |
| for _, database := range []*dbconn.Database{ | ||
| dbconn.Frontend, | ||
| dbconn.CodeIntel, | ||
| } { |
There was a problem hiding this comment.
Note: Still using a few lists like this in some places to use "the same DB for frontend and codeintel" but I will start fixing that in subsequent PRs where I aim to add code-insights-equivilents to these locations.
For now, it at least makes this "we use multiple DBs here which is kinda clunky" clear & explicit
There was a problem hiding this comment.
Yeah I think this is worlds better than tacking a third one on and doing retroactive edge case fixes.
| for _, migrationTable := range dbconn.MigrationTables { | ||
| conds = append(conds, fmt.Sprintf("table_name != '%s'", migrationTable)) | ||
| } | ||
| conds = append(conds, fmt.Sprintf("table_name != '%s'", dbconn.Frontend.MigrationsTable)) |
There was a problem hiding this comment.
lol but what if code intel wants a legit table named codeinsights_schema_migrations????
|
Thanks for the effort here, @slimsag! Separating the codeintel db and the frontend db is a very very low priority at this point as it only affects dev and we have checks on startup to ensure it doesn't actually happen. This PR really minimizes additional muck so I think we can 🚀 with this guy now. |
Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com>
Signed-off-by: Stephen Gutekanst <stephen@sourcegraph.com>
In our DB testing code we make an assumption that we can run all database migrations against a single testing database. This holds true for frontend and codeintel migrations, because we intentionally designed codeintel migrations to be ran in the same DB for testing/dev purposes only - but this does not hold true for Code Insights which runs a separate TimescaleDB instance (it is easier to run it separately in Docker than install it as a Postgres extension in all of our dev/testing environments.)
This change merely makes it so we don't run TimescaleDB migrations against our singleton testing/dev Postgres DB.
Signed-off-by: Stephen Gutekanst stephen@sourcegraph.com