Thanks for the review, Kevin! Please see my response in line.
On 23/04/2018 11:18 AM, Kevin Risden wrote:
* Ran tests at git hash
* Ran tests from tar.gz
* Checked hashes and GPG key of tar.gz
* Reviewed source for missing Apache headers
* Reviewed history and documentation
Few comments:
* Dockerfile - think CMD ["python", "app.py"]" is not needed? line could be
removed
- Done
* go_development.md - docker-compose up - should be "docker-compose up
--build" otherwise might test against old version
- Done
* dsn.go line 191 - typo - principal
- Done
* README.md - dep ensure - Should this point to a specific release? ie:
github.com/apache/calcite-avatica-go@3.0.0
- Dep will search for the latest tag in the repository (in general,
that's what people want). It's possible to force a specific version
using `github.com/apache/calcite-avatica-go@3.0.0`, but I don't think we
need to specifically document that as users of dep will know.
* README.md - godoc - this links to master - didn't see a way to link to a
specific version?
- Godoc only supports master. However, all go projects (including the
golang/go repository) have godoc pointing to master, I think it's best
to follow convention here.
* README.md - Go 1.7+ is supported - are there tests for this? .travis.yml
might be a good place to test multiple go versions
- This is an interesting one. The Go team supports the current version
and the last version. As of today, that would be 1.10.x and 1.9.x. 1.7.x
and 1.8.x are considered obsolete. Most Go projects I use (kubernetes
and docker are using 1.9.x and will move to 1.10.x soon). In 1.9.x, type
aliases were added to the language, so that
https://golang.org/pkg/context/ and
https://godoc.org/golang.org/x/net/context are equivalent. This allows
us to the context library in stdlib without breaking dependencies and
code that uses golang.org/x/net/context.
I will add Go 1.9 to the travis file, but maybe we should just remove
the note that Go 1.7+ is supported to reduce confusion.
* .gitignore - doesn't ignore golang/intellij files
- I've copied all IDE files from Calcite's gitignore into avatica-go's
gitignore.
* goreportcard -
https://goreportcard.com/report/github.com/apache/calcite-avatica-go -
lists some minor things
- These are some minor things we can refactor later :)
None of the above are show stoppers for me.
+1 (non-binding)
Kevin Risden
On Sun, Apr 22, 2018 at 7:05 PM, Francis Chuang <francischu...@apache.org>
wrote:
Hi all,
I have created a release for Apache Calcite Avatica Go 3.0.0, release
candidate 1.
The release notes are available here: https://github.com/apache/calc
ite-avatica-go/blob/master/site/go_history.md
The commit to be voted on: http://git-wip-us.apache.org/r
epos/asf/calcite-avatica-go/commit/273c53f62206c90e6a4fbb05f
4ac82b7f62cb9a7
The hash is 273c53f62206c90e6a4fbb05f4ac82b7f62cb9a7
The artifacts to be voted on are located here:
https://dist.apache.org/repos/dist/dev/calcite/apache-calcit
e-avatica-go-3.0.0-rc1/
The hashes of the artifacts are as follows:
src.tar.gz.md5 00 D1 D0 92 07 C0 60 86 A2 C5 65 E3 B8 60 13 12
src.tar.gz.sha256 6D303B02 442A3999 88DE9234 F0DF733D 49F69B95 35B0C5BC
706F9A14 FAD6F33A
Release artifacts are signed with the following key:
https://people.apache.org/keys/committer/francischuang.asc
Instructions for running the test suite is located here:
https://github.com/apache/calcite-avatica-go/blob/master/
site/go_development.md#testing
Please vote on releasing this package as Apache Calcite Avatica Go 3.0.0.
The vote is open for the next 72 hours and passes if a majority of
at least three +1 PMC votes are cast.
[ ] +1 Release this package as Apache Calcite Avatica Go 3.0.0
[ ] 0 I don't feel strongly about it, but I'm okay with the release
[ ] -1 Do not release this package because...
Here is my vote:
+1 (binding)
Francis
