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

🎉 Containerize connector code generator #3763

Merged

Conversation

TymoshokDmytro
Copy link
Contributor

@TymoshokDmytro TymoshokDmytro commented May 31, 2021

What

#3558 Containerize connector code generator into docker image

How

write Dockerfile to build and run the container that has previous npm script inside
wrap this Dockerfile in generate.sh script to run in one command
Output new connector folder to the connectors directory
Old npm run generate still can be used as previous (Also needed for some addScaffoldTemplateTask)

Pre-merge Checklist

Recommended reading order

./airbyte-integrations/connector-templates/generator/Dockerfile
./generate.sh

@auto-assign auto-assign bot requested a review from michel-tricot May 31, 2021 14:00
@TymoshokDmytro TymoshokDmytro changed the title Dtymoshok/containerize connector code generator 🎉 Dtymoshok/containerize connector code generator May 31, 2021
@TymoshokDmytro TymoshokDmytro changed the title 🎉 Dtymoshok/containerize connector code generator 🎉 Containerize connector code generator May 31, 2021
@TymoshokDmytro
Copy link
Contributor Author

@sherifnada, there is a git file in docs - ./docs/.gitbook/assets/newsourcetutorial_plop.gif
It shows how to use a generator with npm run generate. I think it should be re-posted using the generator.sh script

@yaroslav-dudar
Copy link
Contributor

when I execute generator with npm it produces error if connector with a given name already exists

ydubar@KBP1-LHP-A00649:~/dev/airbyte/airbyte-integrations/connector-templates/generator$ npm run generate
npm WARN lifecycle The node binary used for scripts is /snap/bin/node but npm is using /snap/node/4594/bin/node itself. Use the `--scripts-prepend-node-path` option to include the path for the node binary npm was executed with.

> airbyte-connector-generator@0.1.0 generate /home/ydubar/dev/airbyte/airbyte-integrations/connector-templates/generator
> plop

? [PLOP] Please choose a generator. Python HTTP API Source - Generate a Source that pulls data from a synchronous HTTP API.
? Source name e.g: "google-analytics" fff-1
✖  +! File already exists
 -> /home/ydubar/dev/airbyte/airbyte-integrations/connectors/source-fff-1/Dockerfile
✖  ++ ../../connectors/source-{{dashCase name}}/.dockerignore Aborted due to previous action failure
✖  emitSuccess Aborted due to previous action failure

I suspect that this behavior should be preserved in docker wrapper also

@TymoshokDmytro
Copy link
Contributor Author

@yaroslav-dudar, yes good point. Updated the script to output content to the temporary folder and check if this connector already exists in the connectors directory.

@@ -14,8 +14,7 @@ This document is a general introduction to the CDK. Readers should have basic fa
Generate an empty connector using the code generator. First clone the Airbyte repository then from the repository root run
```
cd airbyte-integrations/connector-templates/generator
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make it run from the project root instead?

./airbyte-integrations/connector-templates/generator/generate.sh

We've had this convention for almost all our scripts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I move the generate.sh to the root of the project, update the script paths, and READMEs. So now it can be started from the airbyte root.

Copy link
Contributor

@tuliren tuliren left a comment

Choose a reason for hiding this comment

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

I have two concerns for this PR.

  1. It only works for source connectors.
  2. It assumes that the generators always copy a set of files to the same location. This is true for all existing generators. However, the plop generator is more powerful than this, and we are actually not using it to the fullest. You can see all the possible actions supported by plop here. One example is that the append action can modify an existing file and add new content to it, which is very useful for registering a new connector.

I have a destination generator PR #3820 that is ready to be merged. That generator won't work with this PR because 1) it adds a destination connector, and 2) it copies files to different locations (code and documentation live under different modules), and modifies multiple files, including generating a UUID and automatically register it in the destination definition yaml file. I also plan to update all existing generators to do this to reduce the number of steps for connector creation.

I would vote against adding the check_and_copy_from_docker, and output the error in another way (maybe checking exit code).

Copy link
Contributor

@tuliren tuliren left a comment

Choose a reason for hiding this comment

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

Actually based on the above comments, I think it's probably better to not merge this PR as is.

generate.sh Outdated Show resolved Hide resolved
@@ -0,0 +1,7 @@
FROM node:14-alpine

RUN mkdir /connectors /sources
Copy link
Contributor

Choose a reason for hiding this comment

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

let's have the same directory structure as we have in the project (airbyte-integrations/connectors)

It will help the 'npm run generate' to log a more helpful path

now it is like:
-> /connectors/source-blaaaaaaaaaaa/Dockerfile
-> /connectors/source-blaaaaaaaaaaa/README.md
-> /connectors/source-blaaaaaaaaaaa/acceptance-test-config.yml
-> /connectors/source-blaaaaaaaaaaa/acceptance-test-docker.sh
-> /connectors/source-blaaaaaaaaaaa/build.gradle
-> /connectors/source-blaaaaaaaaaaa/main.py

Copy link
Contributor Author

@TymoshokDmytro TymoshokDmytro Jun 3, 2021

Choose a reason for hiding this comment

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

✅ Changed it for /airbyte-integrations/connectors/

@TymoshokDmytro TymoshokDmytro linked an issue Jun 3, 2021 that may be closed by this pull request
@midavadim
Copy link
Contributor

I have two concerns for this PR.

  1. It only works for source connectors.
  2. It assumes that the generators always copy a set of files to the same location. This is true for all existing generators. However, the plop generator is more powerful than this, and we are actually not using it to the fullest. You can see all the possible actions supported by plop here. One example is that the append action can modify an existing file and add new content to it, which is very useful for registering a new connector.

I have a destination generator PR #3820 that is ready to be merged. That generator won't work with this PR because 1) it adds a destination connector, and 2) it copies files to different locations (code and documentation live under different modules), and modifies multiple files, including generating a UUID and automatically register it in the destination definition yaml file. I also plan to update all existing generators to do this to reduce the number of steps for connector creation.

I would vote against adding the check_and_copy_from_docker, and output the error in another way (maybe checking exit code).

The mentioned problem can be solved by mounting host file system (whole airbyte root) to the docker. And then it will be able to add/update any files in the project

-v - mount host valumes

docker run -v "$(pwd)":[volume_name] [docker_image]

Copy link
Contributor

@tuliren tuliren left a comment

Choose a reason for hiding this comment

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

The mentioned problem can be solved by mounting host file system (whole airbyte root) to the docker. And then it will be able to add/update any files in the project

Cool. I will address this in #3867.

Thanks for your contribution!

@tuliren tuliren mentioned this pull request Jun 4, 2021
4 tasks
@TymoshokDmytro
Copy link
Contributor Author

Hello @tuliren. Regarding your comments, I've done some updates:
Now the generator can add or modify (or else) files all over the Airbyte project folder.
The main problem with mounting the folder was that generated files have root as an owner.
This problem was solved by translating the UID and GID of the host system user to docker file as an ENV variables and then doing chown on the mounted folder. Please, review this update and let me know if it suits you.

Copy link
Contributor

@tuliren tuliren left a comment

Choose a reason for hiding this comment

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

@TymoshokDmytro, thanks for making the changes. Look good!

Copy link
Contributor

@sherifnada sherifnada left a comment

Choose a reason for hiding this comment

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

Once comment about script location. Once it's addressed please merge!

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

generator_path="$(pwd)/airbyte-integrations/connector-templates/generator"
Copy link
Contributor

Choose a reason for hiding this comment

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

we shouldn't put this in the root path of the repo as to not pollute it. Can we instead put it in airbyte-integrations/connector-templates/generator/generate.sh ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can do this, but michel-tricot in his comment here asked whenever we can start the generator from the project root.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe he meant the same thing based on the path he posted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✅ Done

@TymoshokDmytro TymoshokDmytro merged commit 66d9696 into master Jun 8, 2021
@TymoshokDmytro TymoshokDmytro deleted the dtymoshok/#3558-containerize-connector-code-generator branch June 8, 2021 09:25
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.

Containerize connector code generator into docker image
8 participants