-
Notifications
You must be signed in to change notification settings - Fork 1
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
Upgrade crdb version in workflow #5
Conversation
6edfbbc
to
69ad583
Compare
75db885
to
569142e
Compare
Hi @DoctorVin ,
Our existing tests uses
I am not sure what's happening here. Our new I am guessing it's sqlboiler missing something again which leads to incompatible with latest crdb... Still needs investigation. Anyway, since the latest crdb works well locally. I will de-prioritize upgrading crdb on tests. I'll start working on using latest crdb + fleetdb on sandbox.
|
I agree, we shouldn't be altering generated files by hand. I have a few ideas. Ping me in Slack when you're available. |
569142e
to
67fdab8
Compare
67fdab8
to
35d5327
Compare
35d5327
to
24d7c78
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5 +/- ##
=======================================
Coverage 72.74% 72.74%
=======================================
Files 38 38
Lines 3735 3735
=======================================
Hits 2717 2717
Misses 760 760
Partials 258 258
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Really appreciate @DoctorVin for resolving the sqlboiler and test failures. |
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.
lgtm
Ahhh
dump
is deprecated onv23.1.11
: https://www.cockroachlabs.com/docs/v21.1/cockroach-dumpsqlboiler auto generated code uses
dump
:https://github.com/metal-toolbox/fleetdb/blob/main/internal/models/crdb_main_test.go#L69
Test is failing because of it.