Add Singer infra #58

sherifnada · 2020-08-14T17:37:09Z

What

Allow running tests which use Singer binaries under the hood to verify we can run an entire workflow correctly.

Some complicating factors:

Some Singer connectors depend on binaries installed on the host system e.g: tap-postgres requires pg_config and python3.7.
Testing Singer taps will often also involve setting up other infra components e.g: a Postgres DB for tests

How

We split the server docker image into 2:

The base image: sets up the server's host environment i.e: installs all system dependencies and builds distribution TARs via Gradle. This image is used to run unit and integration tests.
The distribution image: the base image minus the source code

To build the server, we:

build base image
run the base image to run tests
if tests are successful, build the distribution image.
This functionality is now inside ./tools/app/build.sh

The base image is similar to the previous server image with a few notable changes:

It consistently uses the openjdk docker image as the base throughout and manually installs/copies any needed dependencies like gradle to ensure that we're running the exact same OS across build, test, and prod. OpenJDK and the Gradle base used different Linux distros which tripped up Singer system dependencies.
It installs Singer libraries in /usr/local/lib/singer

This PR demos using singer libraries in tests in the class TestSingerDiscoveryWorker. Currently the only test case runs discovery on a Postgres DB which is instantiated inside the test via TestContainer.

Recommended reading order

./tools/app/build.sh
./server_base.Dockerfile
./server_dist.Dockerfile
./tools/singer/setup_singer_env.buster.sh
./tools/singer/install_all_connectors.sh
./tools/singer/install_connectors.sh
TestSingerDiscoveryWorker and BaseWorkerTestCase

There are definitely some more test cases that should be added to TestSingerDiscoveryWorker but I think that should be added in future PRs to keep this change focused on the infra pieces of this more than the test cases.

sherifnada · 2020-08-14T17:39:21Z

build.gradle

@@ -39,6 +39,10 @@ subprojects {

    test {
        useJUnitPlatform()
+        testLogging() {


if tests fail we want to see the reasons on the CLI

sherifnada · 2020-08-14T17:47:35Z

tools/singer/install_all_connectors.sh

+export SINGER_ROOT=$1
+
+pip3 install psycopg2-binary
+./tools/singer/install_connector.sh tap-postgres tap-postgres 0.1.0


pinning versions forces docker to re-install if deps change

sherifnada · 2020-08-14T17:47:55Z

tools/singer/setup_singer_env.buster.sh

+apt-get clean
+apt-get update
+apt-get -y install libpq-dev=11.7-0+deb10u1 \
+  python3.7=3.7.3-2+deb10u2 \


pinning deps for docker caching

sherifnada · 2020-08-14T17:48:44Z

server_base.Dockerfile

+RUN mkdir -p /usr/local/lib/singer
+RUN ./tools/singer/setup_singer_env.buster.sh /usr/local/lib/singer
+
+# Install Node. While the UI is not going to be served from this container, running UI tests is part of the build.


i'm a little unsure why Node is building given this, but added this to pass for now

tools/app/build.sh

cgardens · 2020-08-14T19:24:25Z

dataline-workers/src/test/java/io/dataline/workers/BaseWorkerTestCase.java

@@ -24,23 +28,11 @@ protected Path getWorkspacePath() {

  private void createTestWorkspace() {
    try {
-      workspaceDirectory = Paths.get("/tmp/tests/dataline-" + UUID.randomUUID().toString());
+      workspaceDirectory =
+          Paths.get("/tmp/tests/dataline-" + UUID.randomUUID().toString().substring(0, 8));


Files.createTempDirectory("dataline");?

ah didn't know about that one -- will take a look. TY for the tip.

cgardens · 2020-08-14T19:33:25Z

server_dist.Dockerfile

+# TODO: add data mount instead
+RUN mkdir data
+
+RUN cp /code/dataline-server/build/distributions/*.tar dataline-server.tar \


i'm probably just not understanding something here, but why don't we need to copy the distributions for each module (e.g. scheduler and worker)?

I thought those are both running in the same binary? just thought this was sufficient. We both have the same intenion.

right now there's a single server build artifact

Co-authored-by: Charles <[email protected]>

jrhizor · 2020-08-14T20:13:03Z

tools/app/build.sh

@@ -0,0 +1,19 @@
+#!/usr/bin/env sh


Where is this script triggered? Is it part of the CI process?

michel-tricot

Good starting point but overall looks super hacky

michel-tricot · 2020-08-14T20:49:33Z

dataline-workers/src/test/java/io/dataline/workers/singer/TestSingerDiscoveryWorker.java

+import java.sql.SQLException;
+import java.util.Map;
+import org.junit.jupiter.api.Test;
+import org.testcontainers.containers.PostgreSQLContainer;


I am curious do these test past on github?

michel-tricot · 2020-08-14T20:51:33Z

server_dist.Dockerfile

@@ -0,0 +1,15 @@
+# Build final image
+FROM dataline/server/base:latest


Why do you need to break down the file? Can't you use the multi-stage feature (as we were before)

With the way you split it you're adding a dependency that is not automatic. I can't do docker build server_dist.Dockerfile before doing the same with base

discussed offline:
A couple of reasons:

If tests run during docker build, then all other test dependencies (e.g: postgres container) need to be available + we can't start docker containers from the tests themselves since we can't mount the docker daemon as a volume.

The entrypoint for the container runs tests, and the second container runs the server itself.

We probably want to do something with Docker compose for integration tests in the future.

sherifnada added 14 commits August 13, 2020 17:08

add setup scripts

892731c

add setup scripts

6841917

bionic setup script

b702850

i have prevailed against docker

abef59c

correctly build node

a43a2c0

annotate

b0414dc

locally working test

315a352

save

558e8c9

end to end working

95e0107

touchups

1facf7b

DRY

3217538

remove unused try catch

09c134c

todo

0c57c92

newline

6403b06

sherifnada commented Aug 14, 2020

View reviewed changes

sherifnada requested review from michel-tricot, jrhizor and cgardens and removed request for michel-tricot August 14, 2020 17:49

cgardens approved these changes Aug 14, 2020

View reviewed changes

Update tools/app/build.sh

3077f6d

Co-authored-by: Charles <[email protected]>

jrhizor approved these changes Aug 14, 2020

View reviewed changes

tools/app/build.sh

@@ -0,0 +1,19 @@

#!/usr/bin/env sh

Copy link

Contributor

jrhizor Aug 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is this script triggered? Is it part of the CI process?

michel-tricot approved these changes Aug 14, 2020

View reviewed changes

sherifnada added 6 commits August 14, 2020 14:01

test github wf

8f62231

test

b91aef4

test

45452cd

make executable

2dd3b33

running

09a1d82

remove interactive since we don't need tty

6daa256

sherifnada added 2 commits August 14, 2020 16:06

compare JSON semantically

c49c950

fix merge conflict

03c2817

sherifnada merged commit 636c5dd into master Aug 14, 2020

sherifnada deleted the sherif/singer-infra branch August 17, 2020 17:32

hugoJuhel mentioned this pull request Mar 16, 2022

🎉 Destination mssql : adding ssh tunnel for normalisation #11204

Merged

21 tasks

ptiurin mentioned this pull request Sep 15, 2022

🎉 Base Normalization: Firebolt support #16774

Closed

14 tasks

		@@ -0,0 +1,15 @@
		# Build final image
		FROM dataline/server/base:latest

Add Singer infra #58

Add Singer infra #58

Uh oh!

Conversation

sherifnada commented Aug 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

How

Recommended reading order

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michel-tricot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sherifnada Aug 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sherifnada commented Aug 14, 2020 •

edited

Loading

sherifnada Aug 14, 2020 •

edited

Loading