Impelement oracle database migration

[maxnilz]

Hi, Here is an implementation for oracle DB migration.

Please help to review and see if there is anything need to improve so that we can merge this to master.

Since the Oracle docker image is very big(6GB-8GB) and it will take a long time to do the first start, I'm running a local Oracle docker for the test instead of following the current test routine.

Here are the test results for different Oracle version:

1. Oracle 12.2.0.1-ee(based on image: maxnilz/oracle-ee:12.2.0.1)

...migrate/database/oracle$ ORACLE_DSN=oracle://oracle/oracle@localhost:1521/ORCLPDB1 PKG_CONFIG_PATH=/opt/oracle/instantclient_18_5 LD_LIBRARY_PATH=/opt/oracle/instantclient_18_5 go test -race -v -covermode atomic ./... -coverprofile .coverage
=== RUN   TestParseStatements
--- PASS: TestParseStatements (0.00s)
=== RUN   TestOpen
--- PASS: TestOpen (6.04s)
=== RUN   TestMigrate
--- PASS: TestMigrate (22.72s)
    migrate_testing.go:30: UP
=== RUN   TestLockWorks
=== PAUSE TestLockWorks
=== RUN   TestWithInstance_Concurrent
--- PASS: TestWithInstance_Concurrent (0.64s)
=== CONT  TestLockWorks
--- PASS: TestLockWorks (5.25s)
PASS
coverage: 75.6% of statements
ok  	github.com/golang-migrate/migrate/v4/database/oracle	35.691s	coverage: 75.6% of statements

2. Oracle 18c-xe(based on image maxnilz/oracle-xe:18c)

...migrate/database/oracle$  ORACLE_DSN=oracle://oracle/oracle@localhost:1522/XEPDB1 PKG_CONFIG_PATH=/opt/oracle/instantclient_18_5 LD_LIBRARY_PATH=/opt/oracle/instantclient_18_5 go test -race -v -covermode atomic ./... -coverprofile .coverage
=== RUN   TestParseStatements
--- PASS: TestParseStatements (0.00s)
=== RUN   TestOpen
--- PASS: TestOpen (14.46s)
=== RUN   TestMigrate
--- PASS: TestMigrate (5.37s)
    migrate_testing.go:30: UP
=== RUN   TestLockWorks
=== PAUSE TestLockWorks
=== RUN   TestWithInstance_Concurrent
--- PASS: TestWithInstance_Concurrent (0.51s)
=== CONT  TestLockWorks
--- PASS: TestLockWorks (1.32s)
PASS
coverage: 75.6% of statements
ok  	github.com/golang-migrate/migrate/v4/database/oracle	22.690s	coverage: 75.6% of statement

[maxnilz]

Sorry, I just realized the CI build would fail, because oci8 driver is not a pure golang driver and it needs some extra configuration to make make build-cli works, will try to fix it

[dhui]

Thanks for the PR! I didn't realize that Oracle had publicly available Docker images.

You'll need to remove the bin/migrate file.

Sorry, I just realized the CI build would fail, because oci8 driver is not a pure golang driver and it needs some extra configuration to make make build-cli works, will try to fix it

You can work around this by only enabling Oracle for tests in the Makefile and the Dockerfile. Any other users will need to build migrate and specify support for Oracle.

[maxnilz]

Thanks for the PR! I didn't realize that Oracle had publicly available Docker images.

You'll need to remove the bin/migrate file.

Sorry, I just realized the CI build would fail, because oci8 driver is not a pure golang driver and it needs some extra configuration to make make build-cli works, will try to fix it

You can work around this by only enabling Oracle for tests in the Makefile and the Dockerfile. Any other users will need to build migrate and specify support for Oracle.

Hi, there are some dependencies dynamic libraries missing in the alpine system, So I created a Debian based docker file for the oracle build.

And I introduced an assets dir in oracle for this docker file because It requires to login to the oracle official site & config the license manually for downloading these oracle lib, we can't use wget & curl to download directly.
About the assets dir, I don't think it's a good idea, If there is any other better way please share them out.

[dhui]

I don't have the bandwidth to test this PR manually. e.g. I rely on TravisCI to test PRs
Also, maintaining this driver would also be difficult without automated tests.

So unless there are publicly available Oracle docker images, Oracle won't be supported in migrate.

I really appreciate the work you've done so far and don't want that to go to waste!

Options going forward:

1. We merge this PR after adding language to the README about the lack of official support
2. You host this driver elsewhere and we link to the driver. This would make it more explicit that the driver isn't officially supported.
3. ??? - Any other ideas?

Either way, the official CLI builds (docker and github releases) won't support Oracle due to the lack of automate tests.

If you want to move forward with this PR, the following need to be addressed:

• Remove database/oracle/assets/instantclient-basiclite-linux.x64-18.5.0.0.0dbru.zip e.g. binary blobs don't belong in the source repo
• Rebase/merge from master. Postgres tests are failing which should be fixed now.

[maxnilz]

I don't have the bandwidth to test this PR manually. e.g. I rely on TravisCI to test PRs
Also, maintaining this driver would also be difficult without automated tests.

So unless there are publicly available Oracle docker images, Oracle won't be supported in migrate.

I really appreciate the work you've done so far and don't want that to go to waste!

Options going forward:

1. We merge this PR after adding language to the README about the lack of official support
2. You host this driver elsewhere and we link to the driver. This would make it more explicit that the driver isn't officially supported.
3. ??? - Any other ideas?

Either way, the official CLI builds (docker and github releases) won't support Oracle due to the lack of automate tests.

If you want to move forward with this PR, the following need to be addressed:

• Remove database/oracle/assets/instantclient-basiclite-linux.x64-18.5.0.0.0dbru.zip e.g. binary blobs don't belong in the source repo
• Rebase/merge from master. Postgres tests are failing which should be fixed now.

1. Adding language to the README about the lack of official support, done
2. Host this driver elsewhere and we link to the driver, done
3. Remove database/oracle/assets/instantclient-basiclite-linux.x64-18.5.0.0.0dbru.zip e.g. binary blobs don't belong in the source repo, done
4. Rebase/merge from master. Postgres tests are failing which should be fixed now. done

[tuananh]

any update to this PR?

[maxnilz]

any update to this PR?

Had a quick recap and check, since the oracle released official public docker image(XE express version). will continue this PR and try to send review request again in next few days ASAP

[markussiebert]

Looks promising? Any updates?

[xianchaoyu]

Since the oracle released official public docker image(XE express version), This PR was updated accordingly and Waiting @dhui to Review again

[vtolstov]

any news? also already exists go-ora driver that can be added too.

[dhui]

@maxnilz @xianchaoyu
Hi, sorry for the long delay in review, but life has been busy.

I should have some bandwidth the next couple weeks to review and merge this PR.

Re: go-ora vs godror
I prefer a pure go implementation and not requiring the instant client (a binary blob) but compatibility and support are more important.
Do people have a preference on what the default Oracle driver is? We can always add support for the other later.
This Oracle blog post uses both and the according to that, the main usability difference is the connection string. If that holds true, I lean towards go-ora to avoid the binary blob.

[maxnilz]

@dhui
Hi, thanks for your time.

• I update this PR to use the go-ora already
• For the oracle test cases, since it would take a quite long time to start an oracle container, and starting a container via dktesting and destroying it for each test case is quite a time expense. So I introduced github.com/testcontainers/testcontainers-go to start a container in advance and reused it for all the test cases. BTW, if dktesting can support starting a container once and reusing it many times as needed, I'm very happy to use dktesting instead of introducing this new dependency
• The lint CI is failing because of the network timeout issue, it could be fixed via retry again by someone has the retry permission to trigger it.

Please have a review on these, thanks.

[andrii-glukhyi]

Hi. can we resolve conflicts and merge this in?
Seems like nice feature.
Thanks

[SteffenMahler]

@dhui @xianchaoyu
Could you please be so kind to restart the pipeline. Only the linter had failed. Maybe the PR is as good as done?
It would be really great if migrate would support Oracle.

Thank you

Steffen

[SteffenMahler]

Hi @maxnilz,

thank you for forcing the restart of the pipeline. After that I took a look into the linter problem.

Means, since options is not used for creating the dsn, line 50 of oracle_test.go can be removed completely, then this linter problem is solved.

Edit:
And you have to remove the function param "options" then.

Hope that helps.

Steffen

[xianchaoyu]

Found an issue: Previously I was using the testcontainers-go to start the oracle container for the test cases, and it worked, however After I switched to the dktest, It seems like it couldn't get the port(the port bindings already set) anymore. Will have a check.

[xianchaoyu]

@dhui
I think I found the cause: there is no explicit EXPOSE command in the docker file for the image container-registry.oracle.com/database/express:18.4.0-xe, and in this case, after the dktest create the container, it can not get the port by inspecting the container via the docker client API.

1. The way to fix this is to update the underlying lib. i.e., dktest to set the Config.ExposedPorts in github.com/docker/docker/api/types/container when calling the docker ContainerCreate API
2. As a consumer/user of the dktest we can use it like this

portSet, bindings, _ := nat.ParsePortSpecs([]string{"1521/tcp"})
opts := dktest.Options{
		PortRequired: true,
		ExposedPorts: portSet,
		PortBindings: bindings,
		ReadyFunc:    isReady,
		PullTimeout:  time.Minute * 10,
		Timeout:      time.Minute * 30,
		Mounts:       mounts,
	}

It seems like there is a pending PR dhui/dktest#21 to fix this. Will wait for it to be merged and continue to fix the test cases.

Thanks

[haclark30]

Anything new with this PR?