-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
add chainlink health command; make DB available for testscript client/server tests #11591
Conversation
I see that you haven't updated any README files. Would it make sense to do so? |
f1079e1
to
0f1c3a1
Compare
testdata/scripts/health/health.txtar
Outdated
CL_DATABASE_URL | ||
|
||
-- password -- | ||
T.tLHkcmwePT/p,]sYuntjwHKAsrhm#4eRs4LuKHwvHejWYAC2JP4M8HimwgmbaZ |
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.
:)
6974109
to
fe3581e
Compare
KindEmpty Kind = iota | ||
KindTemplate | ||
KindFixtures |
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.
I always had a hard time reading these funcs, but never looked too closely before. The bool params were not only hard to read, but also mutually exclusive. IMHO it is simpler to combine them in to one option. Before combining, I had just moved them to a separate config struct, so here I ended up settling on a method instead of a func. I considered undoing that, but FWIW this way is nicer for callers outside the package, since it avoids repeating the long package name:
cfg, db := heavyweight.KindEmpty.PrepareDB(t, nil)
vs.
cfg, db := heavyweight.PrepareDB(t, heavyweight.KindEmpty, nil)
te.Setenv("VERSION", static.Version) | ||
te.Setenv("COMMIT_SHA", static.Sha) | ||
|
||
b, err := os.ReadFile(filepath.Join(te.WorkDir, testPortName)) |
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.
why do we need to read a file here? the file name is testPortName
?
does the envVarName
need to be dynamic? does each test get a clean env? can we use something fixed and obvious like ~ TX_TEST_PORT
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.
I needed a mechanism to request these optionally, and I was thinking that more advanced tests could require more than one port or DB, in which case this can easily expand to parse multiple lines 🤷
SonarQube Quality Gate |
// | ||
// -- testdb.txt -- | ||
// CL_DATABASE_URL | ||
testDBName = "testdb.txt" |
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.
i don't see these files in the PR. what's happening? are tests dynamically creating them or something?
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.
They are embedded in the *.txtar
files as described in the comment:
-- testdb.txt --
CL_DATABASE_URL
if err != nil { | ||
return s.errorOut(err) | ||
} | ||
fmt.Println(string(b)) |
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.
is this print intentional? No formatting?
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.
Correct. No formatting necessary. Either it is already plaintext, or they requested raw JSON via the flag anyways. We could pretty print the JSON, but the text version is already the human formatted version of the JSON, and it is trivial for them to use | jq
if they really need that anyways.
Add a
chainlink health
command to print the health status in plaintext or json.Also rigged up database usage for the testscript/txtar tests, so that we can test client/server interaction.