-
Notifications
You must be signed in to change notification settings - Fork 162
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
TrustStore: Implement sqlite database #3467
Conversation
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.
Reviewed 10 of 11 files at r1, 2 of 2 files at r2.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @oncilla)
go/lib/infra/modules/trust/v2/trustdbtest/trustdbtest.go, line 35 at r1 (raw file):
var ( Timeout = time.Second
I wonder if it would be cleaner to pass this in as a config struct. and have default values as constants.
go/lib/infra/modules/trust/v2/trustdbtest/trustdbtest.go, line 47 at r1 (raw file):
on
one
go/lib/infra/modules/trust/v2/trustdbtest/trustdbtest.go, line 50 at r1 (raw file):
// that calls this test-suite. // // Prepare should return a trust database in a clean state, i.e. no entries in the
??? that seems misplaced?
go/lib/infra/modules/trust/v2/trustdbtest/trustdbtest.go, line 74 at r1 (raw file):
// Run test suite on transaction. for name, test := range tests { t.Run(name, func(t *testing.T) {
does that work having 2 sub tests with the same name? I would add something to differentiate test with and without transaction.
go/lib/infra/modules/trust/v2/trustdbtest/trustdbtest.go, line 97 at r1 (raw file):
assert.True(t, inserted) t.Run("TRCExists", func(t *testing.T) { other, err := decoded.DecodeTRC(loadFile(t, "ISD2-V1.trc"))
I think it would be clearer if that were after the comment //Check inexistent
go/lib/infra/modules/trust/v2/trustdbtest/trustdbtest.go, line 120 at r1 (raw file):
assert.Equal(t, dec.TRC, fetched) // Fetch max of existing TRC. max, err := db.GetTRC(ctx, dec.TRC.ISD, scrypto.LatestVer)
I think the test would be slightly better if we'd have a higher version in the DB as well.
go/lib/infra/modules/trust/v2/trustdbtest/trustdbtest.go, line 182 at r1 (raw file):
assert.True(t, issInserted) t.Run("ChainExists", func(t *testing.T) { other, err := decoded.DecodeChain(loadFile(t, "ISD1-ASff00_0_111-V1.crt"))
move to "check nonexistent" block
Add sqlite database that implements the trust.DB Additionally, add a test suite that can test multiple implementations of the trust database.
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.
Reviewed 2 of 2 files at r3.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @oncilla)
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.
Reviewable status: 10 of 14 files reviewed, 7 unresolved discussions (waiting on @lukedirtwalker and @oncilla)
go/lib/infra/modules/trust/v2/trustdbtest/trustdbtest.go, line 35 at r1 (raw file):
Previously, lukedirtwalker (Lukas Vogel) wrote…
I wonder if it would be cleaner to pass this in as a config struct. and have default values as constants.
Done.
go/lib/infra/modules/trust/v2/trustdbtest/trustdbtest.go, line 47 at r1 (raw file):
Previously, lukedirtwalker (Lukas Vogel) wrote…
on
one
Done.
go/lib/infra/modules/trust/v2/trustdbtest/trustdbtest.go, line 50 at r1 (raw file):
Previously, lukedirtwalker (Lukas Vogel) wrote…
??? that seems misplaced?
Done.
go/lib/infra/modules/trust/v2/trustdbtest/trustdbtest.go, line 74 at r1 (raw file):
Previously, lukedirtwalker (Lukas Vogel) wrote…
does that work having 2 sub tests with the same name? I would add something to differentiate test with and without transaction.
Done.
go/lib/infra/modules/trust/v2/trustdbtest/trustdbtest.go, line 97 at r1 (raw file):
Previously, lukedirtwalker (Lukas Vogel) wrote…
I think it would be clearer if that were after the comment
//Check inexistent
Done.
go/lib/infra/modules/trust/v2/trustdbtest/trustdbtest.go, line 182 at r1 (raw file):
Previously, lukedirtwalker (Lukas Vogel) wrote…
move to "check nonexistent" block
Done.
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.
Reviewable status: 10 of 14 files reviewed, 7 unresolved discussions (waiting on @lukedirtwalker)
go/lib/infra/modules/trust/v2/trustdbtest/trustdbtest.go, line 120 at r1 (raw file):
Previously, lukedirtwalker (Lukas Vogel) wrote…
I think the test would be slightly better if we'd have a higher version in the DB as well.
Done.
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.
Reviewed 4 of 4 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved
Add sqlite database that implements the trust.DB
Additionally, add a test suite that can test multiple implementations
of the trust database.
Fixes #3112.
This change is