Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use docker for integrations #87

Merged
merged 31 commits into from
Aug 21, 2020
Merged

Use docker for integrations #87

merged 31 commits into from
Aug 21, 2020

Conversation

michel-tricot
Copy link
Contributor

What

Allows integrations to be implemented as a docker image

How

Images can now be describe in the project.
Singer worker will pull the image when running.
Updated the code to work with images instead of python env.

Recommended reading order

  1. dataline-integrations/singer/*
  2. DockerProcessRunner.java @sherifnada you can remove this file after you're done with the sync. I just left it here so you know how to make it work for your two processes.
  3. Singer*Worker.java

Next steps

@michel-tricot michel-tricot requested review from sherifnada, cgardens and jrhizor and removed request for sherifnada and cgardens August 20, 2020 23:35
Copy link
Contributor

@jrhizor jrhizor left a comment

Choose a reason for hiding this comment

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

Looks good.

How are these going to be built? Separate GitHub projects similar to the Singer structure? I imagine it'd take quite a while to build each of these separately every CI run.

Also, should we be pulling these images in advance for the local build or stop script? Otherwise there will be a big delay before running the job which will be noticeable in the UI for something like running a discovery or a connection check.


public class PostgreSQLContainerHelper {

public static String getSingerConfigJson(PostgreSQLContainer db) throws JsonProcessingException {
return getSingerConfigJson(
db.getUsername(),
db.getPassword(),
db.getHost(),
"host.docker.internal",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this preferable to using the host returned by PostgreSQLContainer / won't this fail in tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because both now run in containers. so the port isn't local to the integration container.

@michel-tricot michel-tricot merged commit d408239 into master Aug 21, 2020
@michel-tricot michel-tricot deleted the mt-isolate-singer branch August 21, 2020 02:32
@sherifnada sherifnada mentioned this pull request Aug 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants