Feedback after first test

airbytehq / ink

Create, build, test and manage your Airbyte connector.

MIT License

1 stars 5 forks source link

Disclaimers:

I only tested until the ./ink run spec command, which is failing.**
I was not able to test ink in docker as you did not push the image to the registry and I did not take the time to build the image myself

Overall feedback

I love how the tool is packaged.
I find that the outputs of the commands are very verbose, in the philosophy of simplifying connector development, I think we should reduce the amount of output sent to sdtout and keep the bare minimum to understand what's going on at a high level.
The tool relies a lot on subprocesses called via python (docker/git/gpg): I'm prone to make "ink in docker" the default runtime to avoid a dependency management hell and control the versions of the required tooling.
I think this is a great prototype to iterate on if we want to make it our default interface for building connectors. The only drawback I find in this approach is, by moving away from the monorepo, user might lose the feeling of being part of an open source project they forked. It might reduce the incentive to contribute. I think we can mitigate this by providing guidance / encouraging messages (or even dedicated a command) to make the connector open source by opening a PR on the monorepo.

Installation

I’d suggest installing the CLI (./ink) globally. Do you find it more convenient to have the wrapper ./ink script within the connector project?
The packaging / install logic is very neat. I think we could take inspiration from it for octavia-cli. Especially for picking python/docker execution environment. Is it a common script packaging pattern I could read more about, or is it something you came up with?
I realize that calling ./ink init actually installs the tool 🤣

`./ink init`

Why is initialize_project calling patch_connector? Shouldn't this function be called only by generate_connector?
The output is verbose due to git clone output being sent to stdout. Wdyt about reducing the amount of output to the strict minimum for understanding what's going on at a high level?

`./ink generate`

--type should be an argument and not an option as its required
We could use a git python client instead of calling git via a subprocess to clone the monorepo
Any strong reason to not run patch_connector before moving its file to the root of the project dir?
Could the generate command also call install?
What do you think of running a git init at the end of generate and creating a first commit with the generated code?
I think that generate does not check that init was run properly. I run once generate before init and got an error saying airbyte.yaml file was missing.
While patching connector you force the python version to be 3.9.0. I do not have this one installed which is making ./ink run fail. Did you patch this temporarily for development convenience on your side?

`./ink install`

The output of generate is really verbose (but this is ./generate.sh fault)
At first sight, the naming of the command looked a bit awkward to me as I was not sure to what install refers to. This is why I suggest running the install of deps on generate .But we'd still need to expose a command to install requirements if the developer changes them.

`./ink run spec`

I could not run this command: FileNotFoundError: [Errno 2] No such file or directory: '/Users/augustin/workspace/source-imr/source_imr/./source_imr/imr.yaml'

I think there is a path manipulation problem somewhere, but I'm not sure if it's related to ink or the declarative source project.

I find that the outputs of the commands are very verbose, in the philosophy of simplifying connector development, I think we should reduce the amount of output sent to sdtout and keep the bare minimum to understand what's going on at a high level.

making it quiet

The tool relies a lot on subprocesses called via python (docker/git/gpg): I'm prone to make "ink in docker" the default runtime to avoid a dependency management hell and control the versions of the required tooling.

agreed but ultimately people will develop on a local env and not on docker so they still need to have a proper version of python installed. We can make the tool docker only, at least for now we can pick one or the other.

I think this is a great prototype to iterate on if we want to make it our default interface for building connectors. The only drawback I find in this approach is, by moving away from the monorepo, user might lose the feeling of being part of an open source project they forked. It might reduce the incentive to contribute. I think we can mitigate this by providing guidance / encouraging messages (or even dedicated a command) to make the connector open source by opening a PR on the monorepo.

Great idea. Monorep was great initially but righ now, especially with how many connectors we have, it is really becoming a bottle neck to contributions. Now I have a "publish" command that is not doing anything, I left it as a next step to decide what publish should mean in the context of Airbyte.

Installation

I’d suggest installing the CLI (./ink) globally. Do you find it more convenient to have the wrapper ./ink script within the connector project?

Much faster onboarding, no dependency on anything or a package manager. (should we use brew, or others?) It makes it very self-contained with a time to implementation very short.

The packaging / install logic is very neat. I think we could take inspiration from it for octavia-cli. Especially for picking python/docker execution environment. Is it a common script packaging pattern I could read more about, or is it something you came up with?

Come up with it :)

I realize that calling ./ink init actually installs the tool 🤣 ./ink init

Why is initialize_project calling patch_connector? Shouldn't this function be called only by generate_connector?

I was using it to play with migration. Removing it.

The output is verbose due to git clone output being sent to stdout. Wdyt about reducing the amount of output to the strict minimum for understanding what's going on at a high level?

Made it quiet

./ink generate

--type should be an argument and not an option as its required

I can't because I want to limit the choice of generators. At least it fails so you know which options you're missing

We could use a git python client instead of calling git via a subprocess to clone the monorepo

What value do we get ?

Any strong reason to not run patch_connector before moving its file to the root of the project dir?

No sure I understand.

Could the generate command also call install?

I can :) done

What do you think of running a git init at the end of generate and creating a first commit with the generated code?

I want to keep the code simple, let wait and see if we should do it

I think that generate does not check that init was run properly. I run once generate before init and got an error saying airbyte.yaml file was missing.

Fixed.

While patching connector you force the python version to be 3.9.0. I do not have this one installed which is making ./ink run fail. Did you patch this temporarily for development convenience on your side?

Yes, to be clear that is one of the weakness of ink right now. I don't know how to manage python.

./ink install

The output of generate is really verbose (but this is ./generate.sh fault)

I can do it at a later time

At first sight, the naming of the command looked a bit awkward to me as I was not sure to what install refers to. This is why I suggest running the install of deps on generate .But we'd still need to expose a command to install requirements if the developer changes them.

You need to have install for when you clone an existing repo. It should be the first command your run after you clone so you have all your deps installed.

./ink run spec

I could not run this command: FileNotFoundError: [Errno 2] No such file or directory: '/Users/augustin/workspace/source-imr/source_imr/./source_imr/imr.yaml' I think there is a path manipulation problem somewhere, but I'm not sure if it's related to ink or the declarative source project.

current problem with the cdk. just remove one of the source_imr, unrelated to ink

airbytehq / ink