Update gorethink from v3 to v6 and rename to proper rethinkdb-go package #1537

cquon · 2020-03-11T01:27:05Z

Gorethink is currently on v6 this is a bump to the v3 client to consume the latest client as well as update required dependencies by the v6 library as well.

cquon · 2020-03-11T02:01:33Z

It appears there are existing linting errors for gosec.

justincormack · 2020-03-11T11:53:44Z

Ah I think gosec did some updates recently and we are not pinning the version.

thaJeztah

Left some comments inline (bear with me, still trying to get used to some oddities of go modules);

Running go mod tidy, I get a diff;

git diff
diff --git a/go.sum b/go.sum
index fe661110..1e02e6e6 100644
--- a/go.sum
+++ b/go.sum
@@ -50,6 +50,7 @@ github.com/golang-sql/civil v0.0.0-20190719163853-cb61b32ac6fe/go.mod h1:8vg3r2V
 github.com/golang/protobuf v1.2.0/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U=
 github.com/golang/protobuf v1.3.4 h1:87PNWwrRvUSnqS4dlcBU/ftvOIBep4sYuBLlh6rX2wk=
 github.com/golang/protobuf v1.3.4/go.mod h1:vzj43D7+SQXF/4pzW/hwtAqwc6iTitCiVSaWz5lYuqw=
+github.com/golang/protobuf v1.3.5 h1:F768QJ1E9tib+q5Sc8MkdJi1RxLTbRcTf8LJV56aRls=
 github.com/google/certificate-transparency-go v1.0.10-0.20180222191210-5ab67e519c93 h1:jc2UWq7CbdszqeH6qu1ougXMIUBfSy8Pbh/anURYbGI=
 github.com/google/certificate-transparency-go v1.0.10-0.20180222191210-5ab67e519c93/go.mod h1:QeJfpSbVSfYc7RgB3gJFj9cbuQMMchQxrWXz8Ruopmg=
 github.com/gorilla/mux v1.7.0 h1:tOSd0UKHQd6urX6ApfOn4XdBMY6Sh1MfxV3kmaazO+U=

We should probably update CI do a go mod tidy ?

On this PR, it also shows this output;

github.com/theupdateframework/notary/server imports
	github.com/prometheus/client_golang/prometheus imports
	github.com/prometheus/common/expfmt imports
	github.com/matttproud/golang_protobuf_extensions/pbutil tested by
	github.com/matttproud/golang_protobuf_extensions/pbutil.test imports
	github.com/golang/protobuf/proto/testdata: module github.com/golang/protobuf@latest (v1.3.5) found, but does not contain package github.com/golang/protobuf/proto/testdata

The testdata package was renamed from testdata to test_proto in this commit: golang/protobuf@8cc9e46

So it looks like go mod checks test files, even though they're not vendored; the vendored code only mentions the gogo/protobuf package, which still has that directory in the version that's vendored;

git grep proto/testdata

vendor/github.com/gogo/protobuf/proto/Makefile: make -C testdata
vendor/github.com/gogo/protobuf/proto/Makefile: protoc-min-version --version="3.0.0" --proto_path=.:../../../../:../protobuf --gogo_out=Mtestdata/test.proto=github.com/gogo/protobuf/proto/testdata,Mgoogle/protobuf/any.proto=github.com/gogo/protobuf/types:. proto3_proto/proto3.proto

Looking at matttproud/golang_protobuf_extensions it indeed references that package; https://github.com/matttproud/golang_protobuf_extensions/blob/v1.0.1/pbutil/all_test.go#L23. That looks to be a bug, because its go.mod requires github.com/golang/[email protected], which does not have the testdata; https://github.com/golang/protobuf/tree/v1.2.0/proto

Ignore this, it's not referencing the package?

I don't expect this to be a problem (because we don't run those tests), but looks like we should open a PR in https://github.com/matttproud/golang_protobuf_extensions to fix.

Some things we should look at:

I see your PR Verify the Pool connection is not nil before attempting to close it rethinkdb/rethinkdb-go#480 was merged, so probably update to v6.1.1, which has that fix
Fix the go.sum (run go mod tidy and commit the results)
update the matttproud/golang_protobuf_extensions to the current release (it's currently 2 commits behind v1.0.0, 4 commits behind v1.0.1); matttproud/golang_protobuf_extensions@d0c3fe8...v1.0.1
Should we update github.com/gogo/protobuf to the current version as well? I think it's usually updated in tandem with the golang/protobuf (current version is v1.3.1, which roughly matches the google/protobuf version)
Perhaps update the golang.org/x/net and golang.org/x/sys packages to current master

Separate from this PR:

Remove the vendor.conf
Fix CI to check go mod tidy (?)
~~Open a PR in https://github.com/matttproud/golang_protobuf_extensions to fix the testdata issue~~ ignore; no idea where it's referenced
Open a PR in https://github.com/rethinkdb/rethinkdb-go to update github.com/cenkalti/backoff to the current (v4) release, which has go module support

thaJeztah · 2020-03-13T08:38:11Z

Makefile

@@ -110,7 +110,7 @@ endif
 	# gosec - requires that the following be run first:
 	#    go get -u github.com/securego/gosec/cmd/gosec/...
 	@rm -f gosec_output.csv
-	@gosec -fmt=csv -out=gosec_output.csv -exclude=G104,G304 ./... || (cat gosec_output.csv >&2; exit 1)
+	@gosec -fmt=csv -out=gosec_output.csv -exclude=G104,G304,G306 ./... || (cat gosec_output.csv >&2; exit 1)


For the record;

G306: Poor file permissions used when writing to a new file

This was to suppress a linting error complaing about a file having 0644 instead of 0600.

nit: probably would've been good to describe this in the commit message, but not a blocker for me

I'll need to make some changes so I'll rebase and update the commit message!

I was trying to remove the vendor.conf and ran into the linting issue, so I carried the first commit in #1540 (and updated the commit message)

once that (and #1539) is merged, you can rebase to have just the changes related to the rethinkdb update in this PR

thaJeztah · 2020-03-13T08:40:32Z

cmd/notary-server/config.go

@@ -25,7 +25,7 @@ import (
 	"github.com/theupdateframework/notary/tuf/signed"
 	"github.com/theupdateframework/notary/utils"
 	"golang.org/x/net/context"
-	"gopkg.in/gorethink/gorethink.v3"
+	gorethink "gopkg.in/rethinkdb/rethinkdb-go.v6"


Curious; didn't it work without the manual aliasing? (I thought anything -go was automatically handled, but perhaps that doesn't work in combination with the .vXX suffix)

No need to change, just curious

No they renamed the package name from gorethink to rethinkdb

https://gopkg.in/rethinkdb/rethinkdb-go.v3
https://gopkg.in/rethinkdb/rethinkdb-go.v6

so figured the path of least resistance was to alias it, also because there is another rethinkdb package in this code that it caused conflict with in some of the files.

oh! for some reason I only saw the -go suffix 🤦‍♂️

thaJeztah · 2020-03-13T09:43:35Z

vendor.conf

+gopkg.in/rethinkdb/rethinkdb-go.v6               bbcde53c1229eece19f8df1ff58cc962e52c0c10 # v6.1.0
+gopkg.in/yaml.v2                    5420a8b6744d3b0345ab293f6fcba19c978f1183 # v2.2.1


We need to remove this file, as it should no longer be used; I noticed that just after the go mod PR was merged; I'll open a PR

PR is up: #1539

(but ran into the gosec linting issue as well, so carried that commit in #1540 😂)

thaJeztah · 2020-03-13T09:49:00Z

go.mod

 	github.com/bugsnag/bugsnag-go v1.0.5-0.20150529004307-13fd6b8acda0
 	github.com/bugsnag/osext v0.0.0-20130617224835-0dd3f918b21b // indirect
 	github.com/bugsnag/panicwrap v0.0.0-20151223152923-e2c28503fcd0 // indirect
-	github.com/cenkalti/backoff v1.0.0 // indirect


Curious why it removes this one from go.mod because the vendor directory still has this dependency vendored. Is it because it's github.com/cenkalti/backoff v2.2.1+incompatible now (no go module support in that version?)

thaJeztah · 2020-03-13T09:53:09Z

go.mod

 	github.com/google/certificate-transparency-go v1.0.10-0.20180222191210-5ab67e519c93 // indirect
 	github.com/gorilla/mux v1.7.0
-	github.com/hailocab/go-hostpool v0.0.0-20160125115350-e80d13ce29ed // indirect


Same for this (just curious); as this one is still vendored

thaJeztah · 2020-03-13T09:54:12Z

go.mod

-	github.com/kr/pretty v0.0.0-20130510082846-bc9499caa0f4 // indirect
-	github.com/kr/text v0.0.0-20130911015532-6807e777504f // indirect


And these (also just curious)

thaJeztah · 2020-03-13T09:57:36Z

go.mod

+	golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3
+	golang.org/x/sys v0.0.0-20191120155948-bd437916bb0e


Should we update these to a more current version? They seem to be somewhat behind

can you do another PR for that?

thaJeztah · 2020-03-13T10:11:38Z

Oh, ignore the mattproud testdata comment; I don't think it's referencing it (looking again)

thaJeztah · 2020-03-13T10:17:10Z

Looks like that issue was fixed in v1.0.1, so updating it to v1.0.1 should address it; matttproud/golang_protobuf_extensions@v1.0.0...v1.0.1

Can you also run go mod vendor ? Because it looks like revendoring with go mod removes a number of files:

root@2f91fcfa08c9:/go/src/github.com/theupdateframework/notary# go mod vendor
root@2f91fcfa08c9:/go/src/github.com/theupdateframework/notary# git status
Refresh index: 100% (1727/1727), done.
On branch gorethink_v6
Changes not staged for commit:
  (use "git add/rm <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

	deleted:    vendor/github.com/google/certificate-transparency-go/x509/root_darwin_arm_gen.go
	deleted:    vendor/github.com/google/certificate-transparency-go/x509/x509_test_import.go
	deleted:    vendor/github.com/lib/pq/oid/gen.go
	deleted:    vendor/golang.org/x/sys/unix/mkasm_darwin.go
	deleted:    vendor/golang.org/x/sys/unix/mkpost.go
	deleted:    vendor/golang.org/x/sys/unix/mksyscall.go
	deleted:    vendor/golang.org/x/sys/unix/mksyscall_aix_ppc.go
	deleted:    vendor/golang.org/x/sys/unix/mksyscall_aix_ppc64.go
	deleted:    vendor/golang.org/x/sys/unix/mksyscall_solaris.go
	deleted:    vendor/golang.org/x/sys/unix/mksysctl_openbsd.go
	deleted:    vendor/golang.org/x/sys/unix/mksysnum.go
	deleted:    vendor/golang.org/x/sys/unix/types_aix.go
	deleted:    vendor/golang.org/x/sys/unix/types_darwin.go
	deleted:    vendor/golang.org/x/sys/unix/types_dragonfly.go
	deleted:    vendor/golang.org/x/sys/unix/types_freebsd.go
	deleted:    vendor/golang.org/x/sys/unix/types_netbsd.go
	deleted:    vendor/golang.org/x/sys/unix/types_openbsd.go
	deleted:    vendor/golang.org/x/sys/unix/types_solaris.go
	deleted:    vendor/golang.org/x/text/unicode/bidi/gen.go
	deleted:    vendor/golang.org/x/text/unicode/bidi/gen_ranges.go
	deleted:    vendor/golang.org/x/text/unicode/bidi/gen_trieval.go
	deleted:    vendor/golang.org/x/text/unicode/norm/maketables.go
	deleted:    vendor/golang.org/x/text/unicode/norm/triegen.go
	modified:   vendor/modules.txt

thaJeztah · 2020-03-13T10:25:09Z

D'oh! That may be because I ran it on Go 1.13. sigh. I hate it that they continue breaking go modules between releases.

thaJeztah · 2020-03-18T12:04:38Z

Looks like it's still failing on that permission error 😞

I also noticed these errors;

find: unknown predicate `-not -path './bower_components/**' -not -path './node_modules/**' -not -path './vendor/**''

Could it be that we're using busybox, and that busybox's find doesn't support these?

cquon · 2020-03-18T14:51:49Z

Looks like it's still failing on that permission error 😞

I also noticed these errors;
find: unknown predicate `-not -path './bower_components/**' -not -path './node_modules/**' -not -path './vendor/**''
Could it be that we're using busybox, and that busybox's find doesn't support these?

This change shouldn't be affecting busybox though right?

cquon · 2020-03-19T04:11:52Z

@thaJeztah I believe the buildscripts/circle-validate-vendor.sh script which invokes buildscripts/validate-vendor.sh ends up touching the go.sum file

Seems to have been a known issue: golang/go#34634 which is likely due to the fact the container is using go 1.12.5. I bumped it to go 1.13 and I saw the file permissions still became readable, but the go mod output was different so I figured it'd be best to stay on the same version of go and manually modify the permissions.

thaJeztah · 2020-03-19T10:30:21Z

buildscripts/validate-vendor.sh

@@ -23,6 +23,8 @@ if [ -d vendor ]; then
  go mod vendor
 fi

+chmod 644 "go.sum"


This must be a bug. Even if go mod ... updates the contents, it should not change file permissions

Yea copied the issue here: golang/go#34634 I think it's resolved in newer versions of go but this is on 1.12

With go 1.12 having reached EOL, should we update to go 1.13 or go 1.14 @justincormack ? I see there's some existing PRs but they appear to be outdated; #1515, #1522, #1536

Could this potentially be merged in to get get updated dependencies working for 1.12 until we can migrate to a newer version of go?

I would rather update Go first.

justincormack · 2020-04-03T09:46:40Z

Go is updated now, this needs a rebase and should fix the go.mod issue.

cquon · 2020-04-03T20:20:54Z

Go is updated now, this needs a rebase and should fix the go.mod issue.

Thanks @justincormack it did indeed 👍

thaJeztah

LGTM (non-binding)

thaJeztah · 2020-04-06T08:27:32Z

@HuKeping ptal 🤗

HuKeping

LGTM!

cquon force-pushed the gorethink_v6 branch 3 times, most recently from d3740f3 to fcb498b Compare March 12, 2020 02:01

thaJeztah suggested changes Mar 13, 2020

View reviewed changes

This was referenced Mar 13, 2020

Remove vendor.conf as it is no longer used #1539

Merged

Fix gosec linting errors (fix G306 and G307) #1540

Merged

update github.com/cenkalti/backoff v4.0.0, and drop Go < 1.12 rethinkdb/rethinkdb-go#481

Merged

cquon force-pushed the gorethink_v6 branch 2 times, most recently from 11bbbcb to 9046bbe Compare March 18, 2020 03:38

cquon force-pushed the gorethink_v6 branch 2 times, most recently from c43a3ba to 2606549 Compare March 19, 2020 04:00

thaJeztah reviewed Mar 19, 2020

View reviewed changes

cquon force-pushed the gorethink_v6 branch 2 times, most recently from d478f15 to 992a3e6 Compare April 3, 2020 19:43

Update gorethink from v3 to v6 and rename to proper rethinkdb-go package

0c5511a

cquon force-pushed the gorethink_v6 branch from 992a3e6 to 0c5511a Compare April 3, 2020 19:45

justincormack approved these changes Apr 5, 2020

View reviewed changes

thaJeztah approved these changes Apr 6, 2020

View reviewed changes

HuKeping approved these changes Apr 6, 2020

View reviewed changes

HuKeping merged commit dc18d79 into notaryproject:master Apr 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update gorethink from v3 to v6 and rename to proper rethinkdb-go package #1537

Update gorethink from v3 to v6 and rename to proper rethinkdb-go package #1537

cquon commented Mar 11, 2020

cquon commented Mar 11, 2020

justincormack commented Mar 11, 2020

thaJeztah left a comment •

edited

Loading

thaJeztah Mar 13, 2020

cquon Mar 14, 2020

thaJeztah Mar 14, 2020

cquon Mar 14, 2020

thaJeztah Mar 13, 2020

cquon Mar 14, 2020

thaJeztah Mar 14, 2020

thaJeztah Mar 13, 2020

thaJeztah Mar 14, 2020

thaJeztah Mar 13, 2020

thaJeztah Mar 13, 2020

thaJeztah Mar 13, 2020

thaJeztah Mar 13, 2020

justincormack Apr 5, 2020

thaJeztah commented Mar 13, 2020

thaJeztah commented Mar 13, 2020

thaJeztah commented Mar 13, 2020

thaJeztah commented Mar 18, 2020

cquon commented Mar 18, 2020

cquon commented Mar 19, 2020 •

edited

Loading

thaJeztah Mar 19, 2020

cquon Mar 19, 2020

thaJeztah Mar 19, 2020

cquon Mar 20, 2020

justincormack Mar 30, 2020

justincormack commented Apr 3, 2020

cquon commented Apr 3, 2020 •

edited

Loading

thaJeztah left a comment

thaJeztah commented Apr 6, 2020

HuKeping left a comment

		gopkg.in/rethinkdb/rethinkdb-go.v6 bbcde53c1229eece19f8df1ff58cc962e52c0c10 # v6.1.0
		gopkg.in/yaml.v2 5420a8b6744d3b0345ab293f6fcba19c978f1183 # v2.2.1

		github.com/kr/pretty v0.0.0-20130510082846-bc9499caa0f4 // indirect
		github.com/kr/text v0.0.0-20130911015532-6807e777504f // indirect

		golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3
		golang.org/x/sys v0.0.0-20191120155948-bd437916bb0e

Update gorethink from v3 to v6 and rename to proper rethinkdb-go package #1537

Update gorethink from v3 to v6 and rename to proper rethinkdb-go package #1537

Conversation

cquon commented Mar 11, 2020

cquon commented Mar 11, 2020

justincormack commented Mar 11, 2020

thaJeztah left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah commented Mar 13, 2020

thaJeztah commented Mar 13, 2020

thaJeztah commented Mar 13, 2020

thaJeztah commented Mar 18, 2020

cquon commented Mar 18, 2020

cquon commented Mar 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justincormack commented Apr 3, 2020

cquon commented Apr 3, 2020 • edited Loading

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah commented Apr 6, 2020

HuKeping left a comment

Choose a reason for hiding this comment

thaJeztah left a comment •

edited

Loading

cquon commented Mar 19, 2020 •

edited

Loading

cquon commented Apr 3, 2020 •

edited

Loading