Modernize bis#404
Conversation
simondeziel
force-pushed
the
modernize-bis
branch
3 times, most recently
from
9da4644 to
e2d3d84
Compare
Signed-off-by: Simon Deziel <simon.deziel@canonical.com>
``` go get -t -v -u ./... # Update gomod dependencies go get golang.org/x/sync@v0.11.0 # Pin to last version supporting old Go, later need 1.23+ go get github.com/mattn/go-sqlite3@v1.14.32 go get github.com/stretchr/testify@v1.11.1 go mod tidy -go=1.22.2 # Enforce minimum Go version go get toolchain@none # Use the bundled toolchain that meets the minimum Go version ``` Signed-off-by: Simon Deziel <simon.deziel@canonical.com>
Signed-off-by: Simon Deziel <simon.deziel@canonical.com>
Signed-off-by: Simon Deziel <simon.deziel@canonical.com>
Signed-off-by: Simon Deziel <simon.deziel@canonical.com>
Signed-off-by: Simon Deziel <simon.deziel@canonical.com>
…eadAll()` Signed-off-by: Simon Deziel <simon.deziel@canonical.com>
Signed-off-by: Simon Deziel <simon.deziel@canonical.com>
Signed-off-by: Simon Deziel <simon.deziel@canonical.com>
Signed-off-by: Simon Deziel <simon.deziel@canonical.com>
Signed-off-by: Simon Deziel <simon.deziel@canonical.com>
Signed-off-by: Simon Deziel <simon.deziel@canonical.com>
Signed-off-by: Simon Deziel <simon.deziel@canonical.com>
Signed-off-by: Simon Deziel <simon.deziel@canonical.com>
Signed-off-by: Simon Deziel <simon.deziel@canonical.com>
simondeziel
force-pushed
the
modernize-bis
branch
from
e2d3d84 to
d7646c9
Compare
marco6
left a comment
marco6
left a comment
There was a problem hiding this comment.
Hi @simondeziel! Thanks for this. Few comments below
| t.Helper() | ||
|
|
||
| dir, err := ioutil.TempDir("", "dqlite-app-test-") | ||
| dir, err := os.MkdirTemp("", "dqlite-app-test-") |
There was a problem hiding this comment.
While modernizing I think we should get rid of this function altogether. Go now has (*T).TempDir which is the better equivalent of this function.
In this sense, it also has (*T).Cleanup which should be used everywhere instead of returning a cleanup function (for example in newAppWithNoTLS).
| for node, metadata := range c.State { | ||
| if node.Role == role && metadata != nil == online { | ||
| if domains == nil || (domains != nil && domains[metadata.FailureDomain]) { | ||
| if domains == nil || domains[metadata.FailureDomain] { |
There was a problem hiding this comment.
This line still has a problem as metadata can bi nil in case online is false
| @@ -22,7 +21,7 @@ const ( | |||
| func bmSetup(t *testing.T, addr string, join []string) (string, *app.App, *sql.DB, func()) { | |||
There was a problem hiding this comment.
Same comment as in the other test. We now have a way for helper functions to register cleanups. In the case of temp dirs, it's even easier. Let's use those features and simplify the flow here.
| t.Helper() | ||
|
|
||
| dir, err := ioutil.TempDir("", "dqlite-replication-test-") | ||
| dir, err := os.MkdirTemp("", "dqlite-replication-test-") |
There was a problem hiding this comment.
Same as other temp dir stuff.
| duration := backoffFunc(attempt) | ||
| // Duration might be negative in case of integer overflow. | ||
| if !(0 < duration && duration <= cap) { | ||
| if 0 >= duration || duration > cap { |
There was a problem hiding this comment.
While not a big deal, is there a reason for this change?
| slot := r.message.lastByte() | ||
| if slot == 0xee { | ||
| switch slot { | ||
| case 0xee: |
There was a problem hiding this comment.
can we name these value as constants given that we are changing this code?
Something like:
const rowsPartMarker = 0xEE;
const rowsDoneMarker = 0xFF;
|
Also @simondeziel, somehow, don't know why the CLA Check is failing. Could you check why? The other PR went through... |
2 participants
Goes hand in hand with canonical/dqlite-ppa#13 now that Focal/20.04 and newer have a golang-1.22-go package available.
This is a reworked version of #396 but that keeps the same OS compatibility (Focal and up).
Also superseds #395 (
mattn/go-sqlite3update).