Move txdb and dialects to chainlink-common/pkg/pg #15064
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
reductionista
force-pushed
the
NONEVM-739/txdb
branch
from
6d01bb7 to
14cfd46
Compare
AER Report: CI Core ran successfully ✅AER Report: Operator UI CI ran successfully ✅ |
reductionista
force-pushed
the
NONEVM-739/txdb
branch
6 times, most recently
from
af2bde1 to
b622cb9
Compare
reductionista
force-pushed
the
NONEVM-739/txdb
branch
from
b622cb9 to
ebe3258
Compare
Flaky Test Detector for
|
reductionista
force-pushed
the
NONEVM-739/txdb
branch
4 times, most recently
from
0dfad7f to
3a606b8
Compare
reductionista
force-pushed
the
NONEVM-739/txdb
branch
4 times, most recently
from
29ac8fa to
b602df5
Compare
reductionista
force-pushed
the
NONEVM-739/txdb
branch
5 times, most recently
from
7752eb4 to
ec9e488
Compare
dhaidashenko
previously approved these changes
Dec 12, 2024
Along with that: - Replace dialects imports with pgcommon - Replace testutils.Skip* with tests.Skip* - Call RegisterTxDb from NewConnection - Run goimports (somehow there were a few files on develop which were improperly formatted?)
While rebasing, I found that CCIP has copied the old txdb driver from chainlink into chainlink/integrations-tests. The problem with duplicated code is it makes it more difficult to update. Little did they know, it had already been updated even before they copied it. So this removes the copy and imports the newer version from chainlink-common. The motivation for copying rather than import appears to have come from a lint rule that enforces not being able to import anything from an external package with "internal" in the directory name. This still requires that the NewSqlxDB be copied, but that's just a wrapper around the pgcommon version which reads the db url from the chainlik toml config, which can't be accessed in chainlink-common.
reductionista
force-pushed
the
NONEVM-739/txdb
branch
from
ec9e488 to
e2f9724
Compare
jmank88
approved these changes
Dec 13, 2024
dhaidashenko
approved these changes
Dec 14, 2024
bolekk
approved these changes
Dec 14, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
NONEVM-739
Requires
smartcontractkit/chainlink-common#910
Description
This mostly just moves
chainlink/core/internal/testutils/pgtest/txdb.goandchainlink/core/store/dialects/dialects.gointo a new common packagechainlink-common/pkg/pg, updating imports accordingly.The purpose of this is so that it can be imported and used by
chainlink-solanafor unit testing Solana ORM's.Since
txdb.gowas a bit of a mess, it has been cleaned up a bit.init()function invoked on package load has been replaced withRegisterTxDb()which accepts a dbUrl as a param instead of automatically reading it from theCL_DATABASEenv var. Since there are only two places in the codebase which depend on our customtxdbdialect of sql being registered, this can simply be called in both of those places (insideNewSqlxDb()andNewConnection()) instead of relying on the globalinit().sql.Register()more than once ifRegisterTxDb()is called more than once (callingsql.Register()more than once with the same driver name will result in an error).context.Backgroundto queries with passing of a new context which gets cancelled when the db is closed. (Note: db here refers to the txdb, which is just a single connection in the lower level pg db, corresponding to the dsn passed in which is a randomly generated UUID.)