Neo4J Support #320

mvid · 2020-01-07T19:05:14Z

Issues to discuss:

The official Neo4J Go driver requires a c library called seabolt to be installed on the user system. That probably isn't acceptable for everyone who wants to work on golang-migrate, so I will need to find some solution, maybe a feature flag to keep the neo code from being loaded in the full test suite, or have it run in a docker container with the library built.
Neo4J doesn't have a DB locking mechanism, so I implemented a local lock using atomic
Neo4J's official driver doesn't pull auth information from the URL, so I am doing that in the Open() function. Ideally the driver would handle this itself.

Neo4J Support in Go Migrate [WIP]

dhui

Thanks for the PR!

The official Neo4J Go driver requires a c library called seabolt to be installed on the user system. That probably isn't acceptable for everyone who wants to work on golang-migrate, so I will need to find some solution

Have you looked into build constraints?

Tests are failing so you'll need to fix them before I can merge your changes.

database/neo4j/neo4j.go

Makefile

dhui

Also, add a README.md and add documentation on how to use the driver. You can use other driver docs (like postgres) as a reference/template

mvid · 2020-01-08T21:18:46Z

@dhui I'm trying to make use of build tags, but I haven't found any docs on limiting to only when a particular library exists. I tried to limit to cgo, but that doesn't seem to be working. Even if I disabled it, we would need to run tests against it somehow, no?

dhui

I'm trying to make use of build tags, but I haven't found any docs on limiting to only when a particular library exists. I tried to limit to cgo, but that doesn't seem to be working. Even if I disabled it, we would need to run tests against it somehow, no?

I'm not very familiar with cgo, so not sure what's involved here. You may need to modify .travis.yml, Makefile, and Dockerfile to get this to build. Neo4j will probably have to be disabled by default in the Makefile unless you want to deal cross compiling. e.g. we build for different OSs and CPU architectures for each release

database/neo4j/README.md

database/neo4j/TUTORIAL.md

database/neo4j/neo4j.go

dhui · 2020-01-09T06:32:47Z

Thanks for addressing all the feedback! All that's left is to get the linter passing, the code building, and check code coverage.

mvid · 2020-01-09T06:43:01Z

I'm trying to make use of build tags, but I haven't found any docs on limiting to only when a particular library exists. I tried to limit to cgo, but that doesn't seem to be working. Even if I disabled it, we would need to run tests against it somehow, no?

I'm not very familiar with cgo, so not sure what's involved here. You may need to modify .travis.yml, Makefile, and Dockerfile to get this to build. Neo4j will probably have to be disabled by default in the Makefile unless you want to deal cross compiling. e.g. we build for different OSs and CPU architectures for each release

I don't think build tags are the solution here, since it isn't really an architecture specific problem, seabolt can be built for all of the currently tested architectures. There are a few considerations, as I see them:

I can add the seabolt dependency to the dockerfile, but then it gets built for everyone instead of just those who plan on using it.
This seems to be the first external c library used in migrate, so CGO_ENABLED=0 will need to be changed.
I have a working docker image that supports the neo4j driver and go, is it possible to run the tests within a specific docker image for specific database types? It's unfortunate that neo4j requires this external library, but I can see other databases having similar limitations in the future

dhui · 2020-01-09T07:00:27Z

I can add the seabolt dependency to the dockerfile, but then it gets built for everyone instead of just those who plan on using it.

That's fine as long as the image isn't too much larger. What's docker image ls show for migrate with neo4j support? The current size is 36.3MB.

This seems to be the first external c library used in migrate, so CGO_ENABLED=0 will need to be changed.

We don't run tests for every OS and arch, so you shouldn't need to disable cgo for tests to pass. But if you want to include neo4j support in the default releases, you'll need to enable cgo and get cross compiling working. We tried to do this with sqlite but had issues.

is it possible to run the tests within a specific docker image for specific database types?

Don't do this. The test environment should be consistent and we don't run tests in a Docker container.

database/neo4j/neo4j.go

coveralls · 2020-01-09T07:42:10Z

Pull Request Test Coverage Report for Build 631

108 of 157 (68.79%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.6%) to 51.991%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
database/neo4j/neo4j.go	108	157	68.79%

Totals
Change from base Build 612:	0.6%
Covered Lines:	2455
Relevant Lines:	4722

💛 - Coveralls

dhui

Cool that you were able to get tests passing! Code coverage looks fine.

Were you able to run make build-cli? Unfortunately, there's no good way to test this in TravisCI. if cross-compiling fails in TravisCI, I'll need to remove neo4j support from the standard releases.

database/neo4j/neo4j.go

mvid · 2020-01-10T06:07:46Z

Cool that you were able to get tests passing! Code coverage looks fine.

Were you able to run make build-cli? Unfortunately, there's no good way to test this in TravisCI. if cross-compiling fails in TravisCI, I'll need to remove neo4j support from the standard releases.

I was getting a
ld: library not found for -lcrt0.o
Which seems to indicate that cross compilation won't work on my mac at least. I think that keeping neo4j out of the standard release is the way to go for now, until the Neo4j team implements a non-cgo driver

What do I need to do to remove it from the standard release?

dhui

Thanks again for addressing my feedback!

...
seems to indicate that cross compilation won't work on my mac at least
...
What do I need to do to remove it from the standard release?

Removing neo4j from DATABASE in the Makefile will prevent it from being released, but it's probably cleaner to add separate DATABASE_TEST variable so tests are run. DATABASE_TEST ?= $(DATABASE) sqlite neo4j should work

dhui · 2020-01-10T06:18:44Z

database/neo4j/neo4j.go

 		}
 	}()

 	result, err := session.Run(string(body[:]), nil)
 	if err != nil {
 		return err
 	}
-	return result.Err()
+	if result.Err() != nil {
+		err = result.Err()


It's cleaner to do:

if e := result.Err(); e != nil { return e } return nil

e.g. there are 2 main concerns:

Don't call Err() multiple times in case the value changes between calls (shouldn't happen, but better safe than sorry)

Explicitly return nil on success (the defer will override any existing value for err)

dhui

Were you able to get docker builds to succeed? I'm hitting the following issue:

Step 7/18 : RUN cmake -D CMAKE_BUILD_TYPE=Release -D CMAKE_INSTALL_LIBDIR=lib .. && cmake --build . --target install
 ---> Running in 97fa7ac53375
-- The C compiler identification is GNU 9.2.0
-- The CXX compiler identification is GNU 9.2.0
-- Check for working C compiler: /usr/bin/cc
-- Check for working C compiler: /usr/bin/cc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Check for working CXX compiler: /usr/bin/c++
-- Check for working CXX compiler: /usr/bin/c++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Building seabolt: 1.7.4-dev
-- Looking for unistd.h
-- Looking for unistd.h - found
-- Check if the system is big endian
-- Searching 16 bit integer
-- Looking for sys/types.h
-- Looking for sys/types.h - found
-- Looking for stdint.h
-- Looking for stdint.h - found
-- Looking for stddef.h
-- Looking for stddef.h - found
-- Check size of unsigned short
-- Check size of unsigned short - done
-- Using unsigned short
-- Check if the system is big endian - little endian
-- Building seabolt with TLS support
-- Using OPENSSL for TLS
-- Looking for timespec_get
-- Looking for timespec_get - found
-- Found OpenSSL: /usr/lib/libcrypto.so (found version "1.1.1d")  
-- Discovered OpenSSL shared libraries: /usr/lib/libssl.so;/usr/lib/libcrypto.so
CMake Error at /usr/share/cmake/Modules/FindPackageHandleStandardArgs.cmake:137 (message):
  Could NOT find OpenSSL, try to set the path to OpenSSL root folder in the
  system variable OPENSSL_ROOT_DIR (missing: OPENSSL_CRYPTO_LIBRARY) (found
  version "1.1.1d")
Call Stack (most recent call first):
  /usr/share/cmake/Modules/FindPackageHandleStandardArgs.cmake:378 (_FPHSA_FAILURE_MESSAGE)
  /usr/share/cmake/Modules/FindOpenSSL.cmake:413 (find_package_handle_standard_args)
  cmake/Internals.cmake:51 (find_package)
  src/seabolt/src/CMakeLists.txt:109 (find_openssl_both)
  src/seabolt/CMakeLists.txt:5 (include)
  src/CMakeLists.txt:14 (include)
  CMakeLists.txt:112 (include)


-- Configuring incomplete, errors occurred!
See also "/seabolt/build/CMakeFiles/CMakeOutput.log".
The command '/bin/sh -c cmake -D CMAKE_BUILD_TYPE=Release -D CMAKE_INSTALL_LIBDIR=lib .. && cmake --build . --target install' returned a non-zero code: 1

mvid · 2020-01-11T14:01:12Z

@dhui I just saw that, looks like something changed recently with openssl. I have updated it now

dhui

Thanks again for another round of fixes! 🙏
Just a few more things to address around building.

Makefile

dhui · 2020-01-11T20:11:54Z

database/neo4j/neo4j.go

+	}
+	defer func() {
+		if cerr := session.Close(); cerr != nil {
+			err = multierror.Append(err, cerr)


Thanks for using multierror. This is much cleaner now!

dhui · 2020-01-11T20:24:53Z

Dockerfile

@@ -3,12 +3,20 @@ ARG VERSION

 RUN apk add --no-cache git gcc musl-dev

+# dependencies for neo4j
+RUN apk add --update --no-cache ca-certificates cmake make g++ libressl-dev openssl-dev git curl pkgconfig


Weird that adding libressl-dev works and using only openssl-dev or libressl-dev doesn't...

It looks like Alpine Linux uses OpenSSL by default since 3.9 so we probably shouldn't be using LibreSSL.

Theoretically, seabolt builds with OpenSSL on Alpine Linux 3.9. Not sure what's different with Alpine Linux 3.11

Let's find the correct variables and values to set so we can use OpenSSL. See:

https://stackoverflow.com/a/16249265

https://github.com/neo4j-drivers/seabolt/blob/1.7/cmake/Internals.cmake#L39

Also, it looks like the built seabolt library needs to be copied over to the final Docker image:

$ docker run --rm migrate/migrate:mvid Error loading shared library libseabolt17.so.1: No such file or directory (needed by /migrate) <OUTPUT TRUCATED> $

Adding COPY --from=downloader /usr/local/lib/libseabolt* /lib/ fixes the issue. I'd this before COPY --from=downloader /go/src/github.com/golang-migrate/migrate/build/migrate.linux-386 /migrate

dhui · 2020-01-11T20:30:49Z

database/neo4j/README.md

@@ -0,0 +1,11 @@
+# neo4j


Add docs about installing seabolt and statically linking OpenSSL to the README

mvid added 9 commits January 6, 2020 15:53

initial pass

729c440

new cli build file

5a1b351

add ready function for docker start

e67feb1

lowercase j

d5340b9

explicit return

79946b1

test migration

a4e7370

undo dockerfile change

f1f063f

Merge pull request #1 from mvid/neo4j

85e0676

Neo4J Support in Go Migrate [WIP]

Merge branch 'master' into master

b4a4116

mvid mentioned this pull request Jan 8, 2020

Add Neo4j Support #321

Closed

dhui requested changes Jan 8, 2020

View reviewed changes

mvid added 5 commits January 9, 2020 02:11

neo4j in dockerfile

2866d25

Merge branch 'master' of github.com:mvid/migrate

ae48550

remove auth from url once transferred to auth token

d1cc5cd

assorted review notes

441db85

initial docs attempt

46a6265

unlock behavior more in line with lock behavior

2050d0f

dhui requested changes Jan 9, 2020

View reviewed changes

database/neo4j/README.md Outdated Show resolved Hide resolved

database/neo4j/TUTORIAL.md Outdated Show resolved Hide resolved

database/neo4j/neo4j.go Show resolved Hide resolved

consistent naming

ea75d4b

linting

e994a5d

seabolt in travis ci

4d636f7

dhui requested changes Jan 9, 2020

View reviewed changes

database/neo4j/neo4j.go Outdated Show resolved Hide resolved

mvid added 3 commits January 9, 2020 12:38

stdlib logger

a0f6bfe

install seabolt as sudo

7124092

enabled cgo

d969cad

correct version #

c468895

mvid requested a review from dhui January 9, 2020 07:44

remove neo4j todo

af4be16

dhui requested changes Jan 9, 2020

View reviewed changes

database/neo4j/neo4j.go Outdated Show resolved Hide resolved

named return values for errors

ce35a58

dhui requested changes Jan 10, 2020

View reviewed changes

mvid added 2 commits January 11, 2020 16:22

review notes

7931609

add libssl for alpine

9936ded

dhui requested changes Jan 11, 2020

View reviewed changes

updates for review

ff92b04

dhui merged commit 3061f45 into golang-migrate:master Jan 15, 2020

Gonzih mentioned this pull request Feb 18, 2020

Bump github.com/golang-migrate/migrate/v4 from 4.8.0 to 4.9.1 Gonzih/poe-status.com#32

Merged

renovate bot mentioned this pull request May 22, 2021

fix(deps): update module github.com/golang-migrate/migrate to v4 - autoclosed advenjourney/advenjourney#20

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Neo4J Support #320

Neo4J Support #320

mvid commented Jan 7, 2020

dhui left a comment

dhui left a comment •

edited

Loading

mvid commented Jan 8, 2020

dhui left a comment

dhui commented Jan 9, 2020

mvid commented Jan 9, 2020

dhui commented Jan 9, 2020

coveralls commented Jan 9, 2020 •

edited

Loading

dhui left a comment

mvid commented Jan 10, 2020

dhui left a comment

dhui Jan 10, 2020

dhui left a comment

mvid commented Jan 11, 2020

dhui left a comment

dhui Jan 11, 2020

dhui Jan 11, 2020

dhui Jan 13, 2020

dhui Jan 15, 2020

dhui Jan 11, 2020

Neo4J Support #320

Neo4J Support #320

Conversation

mvid commented Jan 7, 2020

dhui left a comment

Choose a reason for hiding this comment

dhui left a comment • edited Loading

Choose a reason for hiding this comment

mvid commented Jan 8, 2020

dhui left a comment

Choose a reason for hiding this comment

dhui commented Jan 9, 2020

mvid commented Jan 9, 2020

dhui commented Jan 9, 2020

coveralls commented Jan 9, 2020 • edited Loading

Pull Request Test Coverage Report for Build 631

💛 - Coveralls

dhui left a comment

Choose a reason for hiding this comment

mvid commented Jan 10, 2020

dhui left a comment

Choose a reason for hiding this comment

dhui Jan 10, 2020

Choose a reason for hiding this comment

dhui left a comment

Choose a reason for hiding this comment

mvid commented Jan 11, 2020

dhui left a comment

Choose a reason for hiding this comment

dhui Jan 11, 2020

Choose a reason for hiding this comment

dhui Jan 11, 2020

Choose a reason for hiding this comment

dhui Jan 13, 2020

Choose a reason for hiding this comment

dhui Jan 15, 2020

Choose a reason for hiding this comment

dhui Jan 11, 2020

Choose a reason for hiding this comment

dhui left a comment •

edited

Loading

coveralls commented Jan 9, 2020 •

edited

Loading