Upgrade Go, use modules and enable CI on GitHub Actions #220

errordeveloper · 2021-12-17T14:32:24Z

No description provided.

errordeveloper · 2021-12-17T14:32:49Z

.github/workflows/ci.yaml

+      - run: cd tools && go install github.com/GeertJohan/fgt github.com/mattn/goveralls golang.org/x/lint golang.org/x/tools/cmd/cover
+      - run: script/validate-gofmt
+      - run: go vet ./...
+      # - run: fgt /home/runner/go/bin/lint ./...


I should probably make this work before merging...

errordeveloper · 2021-12-17T14:35:23Z

@thaJeztah do you need to approve the new workflow for it to run on this PR or does need to land in master actually? (I've never contributed GitHub Actions setup to repos I don't have admin access to, so I don't know how is that supposed to work!)

errordeveloper · 2021-12-17T14:37:45Z

See tests results on my fork: https://github.com/errordeveloper/libkv/actions?query=branch%3Aci

thaJeztah · 2021-12-17T14:42:21Z

They don't run until initially merged, but you can create a PR on your own fork to run it (perhaps there's other tricks that @crazy-max knows to do the same though)

crazy-max · 2021-12-17T14:48:34Z

They don't run until initially merged, but you can create a PR on your own fork to run it (perhaps there's other tricks that @crazy-max knows to do the same though)

We must have a dummy workflow on the default branch otherwise it will not triggered. See distribution/distribution#3314

crazy-max · 2021-12-17T14:54:22Z

.github/workflows/ci.yaml

@@ -0,0 +1,44 @@
+name: CI


nit: lowercase/same as workflow filename is preferable (ptsd on my side ahah)

crazy-max · 2021-12-17T14:58:17Z

.travis.yml

@@ -1,7 +1,7 @@
 language: go

 go:
-  - 1.8.7
+  - 1.17.5


I guess your can simply remove the travis file.

I'll just leave it unchanged ;)

On a second thought, yes, probably should just remove it and avoid confusion!

thaJeztah · 2021-12-17T15:21:13Z

go.mod

+
+require (
+	github.com/coreos/etcd v3.3.25+incompatible
+	github.com/go-zookeeper/zk v1.0.2


I wonder if we should decide not (yet) to use a go.mod here, as that would also mean that these dependencies will always be required (without a module, only dependencies for the packages that are actually used will be added in moby (I think?)

Can we move on this PR?

With go1.17 and pruned module graphs, I think this is no longer a issue.

crazy-max · 2021-12-17T15:29:42Z

@errordeveloper #221 is merged. Just rebase and it should work.

Signed-off-by: Ilya Dmitrichenko <errordeveloper@gmail.com>

errordeveloper · 2021-12-17T15:37:22Z

@crazy-max looks like it still refused to run new workflow... let me try to rename it ;)

crazy-max · 2021-12-17T15:38:03Z

@crazy-max looks like it still refused to run new workflow... let me try to rename it ;)

ci.yaml is a new workflow I think that's why it's not triggered. So rename dummy.yml > ci.yml and move the content from your ci.yaml. I think it would work as GitHub will detect changes in that workflow (hopefully).

There had been 4s & 5s timeouts that can lead to test failure, this rounds up both of these to 5s and provide LIBKV_TEST_SHORT_TIMEOUT for CI to override if needed. Signed-off-by: Ilya Dmitrichenko <errordeveloper@gmail.com>

errordeveloper · 2021-12-17T15:41:31Z

@crazy-max somehow that still doesn't work... I think someone just need to approve the run, but maybe we could merge new workflow instead to make it simpler?

crazy-max · 2021-12-17T15:43:07Z

You have created a new dummy.yaml and removed the dummy.yml that's why I think. You have to rename dummy.yml to ci.yml. See #220 (comment).

crazy-max · 2021-12-17T15:44:30Z

Damn no dice :'(

crazy-max · 2021-12-17T15:45:28Z

@errordeveloper Anyway you can check on your fork it seems to fail: https://github.com/errordeveloper/libkv/runs/4562356731?check_suite_focus=true

errordeveloper · 2021-12-17T15:47:27Z

@crazy-max I'll just remove dummy.yml here; I know that it is still failing, seems like a flaky test and I my simple fix didn't work...

errordeveloper · 2021-12-17T16:18:03Z

testutils/utils.go

@@ -136,11 +147,8 @@ func testPutGetDeleteExists(t *testing.T, kv store.Store) {

 func testWatch(t *testing.T, kv store.Store) {


I am really not sure what to do here... sending a sequence of value fixes the test on Consul and Zookeeper, but on etcd updates seem to come out of order :(

Signed-off-by: Ilya Dmitrichenko <errordeveloper@gmail.com>

- make the test more explicit about how watch method currently works on supported stores Signed-off-by: Ilya Dmitrichenko <errordeveloper@gmail.com>

errordeveloper · 2021-12-20T12:31:12Z

The test are passing now!

crazy-max

LGTM

errordeveloper commented Dec 17, 2021

View reviewed changes

errordeveloper force-pushed the ci branch from c11ccde to 1dd4d4f Compare December 17, 2021 14:37

crazy-max mentioned this pull request Dec 17, 2021

Dummy workflow to enable GitHub Actions #221

Merged

crazy-max reviewed Dec 17, 2021

View reviewed changes

errordeveloper force-pushed the ci branch from 94321b2 to af715b6 Compare December 17, 2021 15:01

thaJeztah reviewed Dec 17, 2021

View reviewed changes

Bump Go to 1.17.5 and start using modules

c1a57e7

Signed-off-by: Ilya Dmitrichenko <errordeveloper@gmail.com>

errordeveloper force-pushed the ci branch 2 times, most recently from b003e81 to 96ceae9 Compare December 17, 2021 15:36

errordeveloper force-pushed the ci branch from c5cfdd7 to 4f5dc3f Compare December 17, 2021 15:38

Add env var to override common timeouts in tests

d01e9da

There had been 4s & 5s timeouts that can lead to test failure, this rounds up both of these to 5s and provide LIBKV_TEST_SHORT_TIMEOUT for CI to override if needed. Signed-off-by: Ilya Dmitrichenko <errordeveloper@gmail.com>

errordeveloper force-pushed the ci branch from 4f5dc3f to 24a0409 Compare December 17, 2021 15:40

errordeveloper force-pushed the ci branch from 24a0409 to e3274f3 Compare December 17, 2021 15:44

errordeveloper force-pushed the ci branch 4 times, most recently from a9ea9e6 to 1dba664 Compare December 17, 2021 15:53

errordeveloper force-pushed the ci branch 2 times, most recently from aba7ac6 to 7978b10 Compare December 17, 2021 16:16

errordeveloper commented Dec 17, 2021

View reviewed changes

errordeveloper force-pushed the ci branch 9 times, most recently from 083d82c to 0032462 Compare December 20, 2021 12:23

errordeveloper added 3 commits December 20, 2021 12:27

Remove Travis CI

8e504cf

Signed-off-by: Ilya Dmitrichenko <errordeveloper@gmail.com>

Configure basic GitHub Actions workflow

4ba2497

Signed-off-by: Ilya Dmitrichenko <errordeveloper@gmail.com>

Rewrite testWatch

45790e4

- make the test more explicit about how watch method currently works on supported stores Signed-off-by: Ilya Dmitrichenko <errordeveloper@gmail.com>

errordeveloper force-pushed the ci branch from 0032462 to 45790e4 Compare December 20, 2021 12:28

crazy-max approved these changes Dec 23, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade Go, use modules and enable CI on GitHub Actions #220

Upgrade Go, use modules and enable CI on GitHub Actions #220

errordeveloper commented Dec 17, 2021

errordeveloper Dec 17, 2021

errordeveloper commented Dec 17, 2021

errordeveloper commented Dec 17, 2021

thaJeztah commented Dec 17, 2021

crazy-max commented Dec 17, 2021 •

edited

Loading

crazy-max Dec 17, 2021 •

edited

Loading

crazy-max Dec 17, 2021

errordeveloper Dec 17, 2021

errordeveloper Dec 17, 2021

thaJeztah Dec 17, 2021

zhsj Apr 4, 2022 •

edited

Loading

crazy-max commented Dec 17, 2021

errordeveloper commented Dec 17, 2021

crazy-max commented Dec 17, 2021

errordeveloper commented Dec 17, 2021

crazy-max commented Dec 17, 2021

crazy-max commented Dec 17, 2021

crazy-max commented Dec 17, 2021

errordeveloper commented Dec 17, 2021 •

edited

Loading

errordeveloper Dec 17, 2021

errordeveloper commented Dec 20, 2021

crazy-max left a comment

		@@ -136,11 +147,8 @@ func testPutGetDeleteExists(t *testing.T, kv store.Store) {

		func testWatch(t *testing.T, kv store.Store) {

Upgrade Go, use modules and enable CI on GitHub Actions #220

Are you sure you want to change the base?

Upgrade Go, use modules and enable CI on GitHub Actions #220

Conversation

errordeveloper commented Dec 17, 2021

errordeveloper Dec 17, 2021

Choose a reason for hiding this comment

errordeveloper commented Dec 17, 2021

errordeveloper commented Dec 17, 2021

thaJeztah commented Dec 17, 2021

crazy-max commented Dec 17, 2021 • edited Loading

crazy-max Dec 17, 2021 • edited Loading

Choose a reason for hiding this comment

crazy-max Dec 17, 2021

Choose a reason for hiding this comment

errordeveloper Dec 17, 2021

Choose a reason for hiding this comment

errordeveloper Dec 17, 2021

Choose a reason for hiding this comment

thaJeztah Dec 17, 2021

Choose a reason for hiding this comment

zhsj Apr 4, 2022 • edited Loading

Choose a reason for hiding this comment

crazy-max commented Dec 17, 2021

errordeveloper commented Dec 17, 2021

crazy-max commented Dec 17, 2021

errordeveloper commented Dec 17, 2021

crazy-max commented Dec 17, 2021

crazy-max commented Dec 17, 2021

crazy-max commented Dec 17, 2021

errordeveloper commented Dec 17, 2021 • edited Loading

errordeveloper Dec 17, 2021

Choose a reason for hiding this comment

errordeveloper commented Dec 20, 2021

crazy-max left a comment

Choose a reason for hiding this comment

crazy-max commented Dec 17, 2021 •

edited

Loading

crazy-max Dec 17, 2021 •

edited

Loading

zhsj Apr 4, 2022 •

edited

Loading

errordeveloper commented Dec 17, 2021 •

edited

Loading