Add snapshot database schema #10358
Add snapshot database schema #10358
Conversation
5ce9746 to
d19550e
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #10358 +/- ##
==========================================
+ Coverage 62.18% 62.22% +0.04%
==========================================
Files 1219 1221 +2
Lines 87663 87706 +43
Branches 1820 1820
==========================================
+ Hits 54513 54578 +65
+ Misses 32310 32290 -20
+ Partials 840 838 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
d19550e to
6bdc734
Compare
size-limit report 📦
|
| snapID, err = createSnapshotRow(ctx, tx, s) | ||
| return errors.Wrap(err, "createSnapshotRow") | ||
| }); err != nil { | ||
| t.Fatalf("WithTx: %v", err) |
There was a problem hiding this comment.
Why not use require.NoError here and the following error checks? It can also pass in a message
There was a problem hiding this comment.
There was a problem hiding this comment.
There are so many require in our code base. So when to use require and when to use Fatalf?
There was a problem hiding this comment.
I'd say be consistent with the existing tests in the file. If starting a new file, avoid require.
There was a problem hiding this comment.
Got it.
why did we have require?
There was a problem hiding this comment.
People coming from other languages expect to write tests like that, so they added it to the codebase so they could continue doing things their own way instead of the go way. At the end of the day it doesn't really matter, but a lot of our tests abort too soon (meaning you need to run it multiple times to gradually fix an error, rather than being able to confidently have a fixed test after reading the output) because of the require sugar layer. (The open source library we "copied" has "assert" and "require" that Errorf or Fatalf depending on which one was used. This would avoid the bailout-early test.)
The other problem is that people write things like require.Equal(want, len(got)) which prints a message like "want: 1, got: 2" which doesn't answer the follow-up question "ok well what was in got". Hand-written tests tend to not make this mistake.
In general you should use cmp.Diff in these cases as the diff always explains the failure as deeply as necessary to fix the test. There is also require.NoDiff which I don't mind people using in new code, though it's a little uglier than manually calling cmp.Diff in my opinion (and often kills the test prematurely, again because require's thing is to end the test case with Fatalf on failure).
There was a problem hiding this comment.
This might be useful to be brought up in a team meeting
| t.Fatalf("writer.Close: %v", err) | ||
| } | ||
| if err := dbutil.WithTx(ctx, db, func(ctx context.Context, tx *pachsql.Tx) error { | ||
| return errors.Wrap(addDatabaseDump(ctx, tx, snapID, *fsID), "addDatabaseDump") |
There was a problem hiding this comment.
Should we also read from database to make sure they are properly updated?
There was a problem hiding this comment.
It would be nice, but we don't have any functions to read the rows yet and this PR doesn't need to add them. Really all we're trying to check here is that the foreign key works and doesn't cause an error when adding a presumably-valid fileset id to the row.
There was a problem hiding this comment.
Actually, I added a test for this.
| type SnapshotID int64 | ||
|
|
||
| // String implements fmt.Stringer. | ||
| func (id SnapshotID) String() string { |
There was a problem hiding this comment.
For my understanding: Will this String() be ever passed in a id<1? Considering that is invalid and should have raised error in upper stack.
There was a problem hiding this comment.
It's meant to store a postgres bigserial type, which is an integer from 0 to 2^63-1, but there is no type in Go that has those constraints. No matter type we pick on the Go side, there will be values that can't be used as row IDs. (The alternative is to use uint64 and make [2^63, 2^64-1] the invalid ids.)
2e7aca1 to
c43679e
Compare
|
PTAL. |
This adds the basic snapshot database schema, and a package to put the business logic in.
I have stubbed out some steps for creating snapshots, just to ensure the foreign keys work as expected.
I also added a function to dockertestenv to return a database with migrations already applied. This pattern is cut-n-pasted into a ton of places. I'll clean that up in the near future.