Move tools/ci_* projects to airbyte-ci, update to use Poetry, bump to python 3.10 #27957

bnchrch · 2023-07-04T21:34:24Z

What

moves tools/ci_* projects to airbyte-ci
Updates them to use poetry/pipx
Updates these packages to python 3.10
Improves the installation story
Moves pipelines to its own package

Now to install airbyte-ci all you have to run is

pipx install airbyte-ci/connectors/pipelines

Notes for reviewer

@alafanechere I would love to get your take on this being the airbyte-ci tzar.

Is this a good idea?
Is this the direction you had wanted to go in?
What do you think about using pipx to isolate the install?
The folders ci and connectors at the top of airbyte-ci still feel awkward is there a better way?

Left TODO

closes #25170
closes #27865

bnchrch · 2023-07-05T06:46:30Z

airbyte-ci/ci/ci_pipelines/README.md

Pipelines/airbyte-ci are now their own package

bnchrch · 2023-07-05T06:46:59Z

airbyte-ci/ci/ci_pipelines/README.md

+Once pipx is installed, navigate to the root directory of the project, then run:
+
+```bash
+pipx install airbyte-ci/ci/ci_pipelines/


The installation is one line

bnchrch · 2023-07-05T06:47:28Z

airbyte-ci/ci/ci_credentials/pyproject.toml

+packages = [{ include = "ci_credentials" }]
+
+[tool.poetry.dependencies]
+python = "^3.10"


Python 3.10 all around!!!

ci_credentials is installed on GHA workflows that are running 3.9. Can we keep compatibility of ci_credentials with 3.9 while suggesting use of 3.10 though .python-version files?

I believe we can!

Is there a reason to keep the version diff between local and ci? Could this be another source of behavior mismatches?

nvm, seems like we bumped most things as we went along, except .github/workflows/connector-performance-command.yml for some reason?

Oh I think that was a miss on my part.

I ended up going with 3.10 everywhere as it seems that there were no issues encountered while running these tools as 3.10

bnchrch · 2023-07-05T06:48:01Z

airbyte-ci/ci/ci_credentials/pyproject.toml

+requests = "^2.28.2"
+click = "^8.1.3"
+pyyaml = "^6.0"
+ci_common_utils = { path = "../ci_common_utils", develop = true }


If we go ahead with this we will need to update the local dependency resolution right?

alafanechere · 2023-07-05T08:54:47Z

Is this a good idea?

Yes it is!

Is this the direction you had wanted to go in?

Totally

What do you think about using pipx to isolate the install?

I'm not against it but I'm not familiar with it. Is it providing dependency isolation while not requiring the use of a python virtualenv? I'm 👍 , but if poetry has a builtin way to achieve the same I'd prefer to stick to poetry (I'm not too familiar with it too..)

The folders ci and connectors at the top of airbyte-ci still feel awkward is there a better way?

So in this ci_connector_ops / ci_credentials / ci_common_utils thing we have:

QA checks
airbyte-ci pipelines
ci_credentials to download GSm secrets
ci_common_utils which only has a GoogleAPI client class... Which I believe is only used by ci_credentials.

My suggestion would be to:

Merge ci_credentials and ci_common_utils into a single package : airbyte-ci/connectors/credentials ?
I'd love to have a package we can use as a lib that would have the Connector utility class. airbyte-ci/connectors/utils ? It could be imported in pipelines and qa_checks for instance.
The QA checks should be a light package in airbyte-ci/connectors/qa_checks

bnchrch · 2023-07-05T15:51:58Z

@alafanechere Thanks for the swift reply and glad to hear this is in the same direction you had in mind!

if poetry has a builtin way to achieve the same I'd prefer to stick to poetry

Poetry doesntly have a good way to "install" these cli tools.

So we'd have to run the airbyte-ci by

cd airbyte-ci/ci/ci_pipelines
poetry install
poetry run airbyte-ci

Note: we'd also have to update all the paths to work from this folder not root

However thats where pipx comes in

its purpose built to install cli tools in an isolated way. This should let us have our customers (connector devs) install airbyte-ci ci-credentials and any other tool completely isolated without having conflicts.

It also interfaces with pyproject.toml and the lockfiles perfectly!

My suggestion would be to:

Merge ci_credentials and ci_common_utils into a single package : airbyte-ci/connectors/credentials ?

I'd love to have a package we can use as a lib that would have the Connector utility class. airbyte-ci/connectors/utils ? It could be imported in pipelines and qa_checks for instance.

The QA checks should be a light package in airbyte-ci/connectors/qa_checks

These all sound great. Would you mind if we moved the refactor of ci_common_utils and ci_credentials to later PR?

Just so I can get this in with minimal conflicts 😅

What I can do is rename these to remove ci and place them all under airbyte-ci/connectors/

e.g.
airbyte-ci/connectors/common_utils
airbyte-ci/connectors/pipelines
airbyte-ci/connectors/credentials

alafanechere · 2023-07-05T16:02:56Z

Allright, let's use pipx!
And let's refactor ci_common_utils and ci_credentials in a different PR!

What I can do is rename these to remove ci and place them all under airbyte-ci/connectors/

👍

I'd love to have a package we can use as a lib that would have the Connector utility class. airbyte-ci/connectors/utils ? It could be imported in pipelines and qa_checks for instance.

Do you have an opinion about this?

bnchrch · 2023-07-05T16:35:50Z

I'd love to have a package we can use as a lib that would have the Connector utility class. airbyte-ci/connectors/utils ? It could be imported in pipelines and qa_checks for instance.

Do you have an opinion about this?

I LOVE this. Thats my opinion. I think were in a great spot to do that. Also it would be useful in the metadata_service

…ipelines

octavia-squidington-iii · 2023-07-25T01:46:56Z

source-faker test report (commit `26884cd0db`) - ❌

⏲️ Total pipeline duration: 03mn30s

Step	Result
Validate airbyte-integrations/connectors/source-faker/metadata.yaml	✅
Connector version semver check	✅
Connector version increment check	❌
QA checks	✅
Code format checks	✅
Connector package install	✅
Build source-faker docker image for platform linux/x86_64	✅
Unit tests	✅
Acceptance tests	✅

🔗 View the logs here

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-faker test

bnchrch · 2023-07-25T01:49:23Z

airbyte-ci/connectors/ci_code_validator/__init__.py

@airbytehq/connector-operations Gut check: We should remove this.

There is

no use of ci_sonar_qube in code

no use of ci_changes_detection in code

the is one use of ci_sonar_qube in a github action: https://github.com/airbytehq/airbyte/pull/27957/files#diff-c46f6e02d0050829e4e9f4a4bd0eb29da35853e0786b7ddf348f665085c561da

But that action is not used.

I've never heard of this, if that helps anything!

bnchrch · 2023-07-25T01:50:00Z

.github/workflows/test-performance-command.yml

This should be deleted.

See: https://github.com/airbytehq/airbyte/pull/27957/files#r1272911875

bnchrch · 2023-07-25T01:59:45Z

@airbytehq/connector-operations This is ready for review!

Most of this is

a folder restructure
a tooling change
a python version bump

The only file with net new logic is how we install pipx and use it in the pipelines here

...tor_ops/pipelines/actions/environments.py → ...pelines/pipelines/actions/environments.py

Please let me know what you think, I would like this to be a speedy merge to avoid conflicts!

erohmensing

Installation was a breeze! Messed around with formatting a source, testing the orchestrator and metadata lib. I'm still not sure (via READMEs alone, and after a quick non-deep perusal of CI changes) how to run tests for the pipelines.

Some non-blocking feedback - I think we should merge this improvement before trying to tweak anything much more:

I was able to run the QA engine from where it lives with the instructions provided. I find it sort of weird it lives under airbyte ci but has a separate installation and run process
It looks like the README of airbyte-ci lives under airbyte-ci/connectors/pipelines - i'd expect it to live under airbyte-ci or maybe airbyte-ci/connectors. I guess this makes sense because of the install command being pipx install airbyte-ci/connectors/pipelines... this installation path confuses me since i expect airbyte-ci to be able to run the metadata service as well (and the qa engine as described above)

erohmensing · 2023-07-25T13:31:23Z

airbyte-ci/connectors/ci_code_validator/__init__.py

I've never heard of this, if that helps anything!

alafanechere

Approving to unblock but could you please checkout the report path problem and the dev mode story? 😄

alafanechere · 2023-07-25T17:38:53Z

airbyte-ci/connectors/pipelines/pyproject.toml

+
+[tool.poetry.dependencies]
+python = "^3.10"
+dagger-io = "^0.6.4"


Before installing airbyte-ci with pipx I've set my current Python version to Python 3.10.8 with pyenv:
pyenv shell 3.10.8.
In this python version I already had installed dagger-io in a different version: 0.5.4.
When I ran pipx install airbyte-ci/connectors/pipelines/ it did not update the dagger version.

When I retried without pyenv shell the install worked correctly and the correct version is used.

As I understand it. This is expected!

https://github.com/pypa/pipx#how-is-it-different-from-pip

pipx actually installs all dependencies for the specified package in a directory isolated from your current python environment.

The packages you have in you global env or pyenv will have no effect on the package dependencies install with pipx and vice versa

So it makes sense to me that the dagger-io version in your pyenv shell didnt change.

But can you tell me what you ran to get the version for dagger-io?

I think the pipx behavior is great when you're not using pyenv shell before:
When you use pyenv shell 3.10.8 I think that pyenv does also create/reuse a venv for the specific python version you use.
In my context, my pyenv venv had an old dagger-io version that I installed with pip install dagger-io
When I ran pipx install after running pyenv shell 3.10.8 the install works but when running airbyte-ci the "old" dagger version was used and not the one declared in airbyte-ci.

It's a strange behavior I can't really explain: my suggestion is just to keep in mind that the pyenv shell + pipx combo can lead to strange install and not suggest developer to use pyenv shell.
TLDR: don't change anything on the doc, the classic install path look stable, but in case of installation issue by developers we should just keep in mind to ask them how they installed and use Python 3.10.8.

alafanechere · 2023-07-25T17:42:33Z

airbyte-integrations/bases/connector-acceptance-test/acceptance-test-docker.sh

@@ -16,7 +16,8 @@ CONNECTOR_DIR="$ROOT_DIR/airbyte-integrations/connectors/$CONNECTOR_NAME"

 if [ -n "$FETCH_SECRETS" ]; then
  cd $ROOT_DIR
-  VERSION=dev $ROOT_DIR/tools/.venv/bin/ci_credentials $CONNECTOR_NAME write-to-storage || true
+  pipx install airbyte-ci/connectors/ci_credentials


Can we make this script install pipx directly to make it self contained?

alafanechere · 2023-07-25T17:47:48Z

airbyte-ci/connectors/pipelines/README.md

@@ -1,30 +1,50 @@
-# Airbyte CI CLI 
+# Airbyte CI CLI


Can we add a link in the airbyte-ci/README.md to this file?

alafanechere · 2023-07-25T17:59:20Z

airbyte-ci/connectors/pipelines/README.md

 ```

+This command installs `airbyte-ci` and makes it globally available in your terminal.


I think folks should open a new terminal or source their bash profile to get airbyte-ci in their path.

Could you elaborate a bit more on the why and what an ideal install instruction looks like?

I think that after you run pipx install the airbyte-ci the CLI is not directly available in the PATH.
I suggest to add something like: Open a new terminal session or source your bash profile to make airbyte-ci available in your path.

Hmm it should be immediately available on your path 🤔

It wasnt for you?

alafanechere · 2023-07-25T18:06:13Z

airbyte-ci/connectors/pipelines/README.md

+If you are developing on pipelines, we recommend installing airbyte-ci in editable mode:
+
+```bash
+pipx install airbyte-ci/connectors/pipelines/ --force --editable
+```


I'm a bit skeptical about this approach for dev mode.
When developping on airbyte-ci I'm used to iterate in a virtual environment and have clear output of the dependencies installed when in run pip install. When using pipx install it's not obvious where is this environment located and how it would clash with an other non editable install.
I installed in editable mode, changed a dependency version, reinstall in editable mode: the dependency was not upgraded.
To check what dependencies are install I use pip freeze. In this context, as I'm not in a virtual environment the pip freeze outputs my globally installed packages, not the airbyte-ci specific one.

TLDR: in dev mode I'd like to explicitely switch to a python virtualenv and get an easy way to check what deps are installed.

The solution here is probably to use poetry instead of pipx for development:

pyenv shell 3.10.8 # I can't make poetry work with python 3.9, I face this issue https://github.com/python-poetry/poetry/issues/7695 poetry install # this creates a virtualenv (.venv) in the package folder poetry shell # activates the virtualenv cd ../../.. airbyte-ci

Thumbs up to using poetry for dev!

Seems a lot more approachable and intuitive :)

alafanechere · 2023-07-25T18:14:32Z

airbyte-ci/connectors/pipelines/pipelines/consts.py

@@ -32,6 +32,6 @@
 GRADLE_CACHE_PATH = "/root/.gradle/caches"
 GRADLE_BUILD_CACHE_PATH = f"{GRADLE_CACHE_PATH}/build-cache-1"
 GRADLE_READ_ONLY_DEPENDENCY_CACHE_PATH = "/root/gradle_dependency_cache"
-LOCAL_REPORTS_PATH_ROOT = "tools/ci_connector_ops/pipeline_reports/"
+LOCAL_REPORTS_PATH_ROOT = "airbyte-ci/connectors/pipelines/pipeline_reports/"


This path looks good, but when I execute an empty pipeline_reports folder is created at:

airbyte-ci/airbyte-ci/connectors/pipelines/pipeline_reports

And the actual test reports are written a very nested location:

airbyte-ci/connectors/pipelinespipeline_reports/airbyte-ci/connectors/test/manual/bnchrch_ci_poetry-pipelines/1690300788/26884cd0dbe194727b5982e7b67c96e7b60573a6/dagger.log

Something is probably missing up with the path in reports.py or context.py:

local_path = anyio.Path(f"{LOCAL_REPORTS_PATH_ROOT}/{self.report_output_prefix}/{filename}") await local_path.parents[0].mkdir(parents=True, exist_ok=True)

Ooh good catch. Let me see whats going on

Ah I see whats going on

empty pipeline_reports folder created
This is an is issue with where we create the report directory which is only discovered if you run the airbyte-ci cmd not in the root.

Ill move this closer to us writing the log

And the actual test reports are written a very nested location:
It is very nested but expected

The log path airbyte-ci/connectors/pipelines/pipeline_reports/airbyte-ci/connectors/test/manual/bnchrch_ci_poetry-pipelines/1690300788/26884cd0dbe194727b5982e7b67c96e7b60573a6/dagger.log has three discrete parts

The parent folder airbyte-ci/connectors/pipelines/

The airbyte-ci defined path structure airbyte-ci/connectors/test/

the git info manual/bnchrch_ci_poetry-pipelines/1690300788/26884cd0dbe194727b5982e7b67c96e7b60573a6

so you will note that #1 and #2 are nearly identical.

and that turns out is a coincidence.

The airbyte-ci defined path structure is derived from the click command hierarchy airbyte-ci connectors test --name=...

alafanechere · 2023-07-25T18:22:23Z

airbyte-ci/connectors/pipelines/pyproject.toml

+requests = "^2.28.2"
+connector-ops = {path = "../connector_ops"}
+toml = "^0.10.2"
+


Could you add some testing dependencies? (feel free to copy the one that were in the main setup.py

…ipelines

octavia-squidington-iii · 2023-07-26T01:45:16Z

source-faker test report (commit `68758f99ba`) - ❌

⏲️ Total pipeline duration: 03mn22s

Step	Result
Validate airbyte-integrations/connectors/source-faker/metadata.yaml	✅
Connector version semver check	✅
Connector version increment check	❌
QA checks	✅
Code format checks	✅
Connector package install	✅
Build source-faker docker image for platform linux/x86_64	✅
Unit tests	✅
Acceptance tests	✅

🔗 View the logs here

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-faker test

bnchrch · 2023-07-26T01:48:01Z

@alafanechere @pedroslopez

Just updated the branch to

Have poetry based dev install instructions
Have pipeline test deps declared
Remove ci_code_validator folder and related action
Upgrade .github/workflows/connector-performance-command.yml to 3.10
Ran the python formatter on airbyte-ci
Updated the logging code to not create the dagger.log parent folder until nessesary

Im going to run some test pipelines now to ensure everything works as planned

octavia-squidington-iii · 2023-07-26T01:51:49Z

source-faker test report (commit `4090391bac`) - ❌

⏲️ Total pipeline duration: 03mn17s

Step	Result
Validate airbyte-integrations/connectors/source-faker/metadata.yaml	✅
Connector version semver check	✅
Connector version increment check	❌
QA checks	✅
Code format checks	✅
Connector package install	✅
Build source-faker docker image for platform linux/x86_64	✅
Unit tests	✅
Acceptance tests	✅

🔗 View the logs here

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-faker test

bnchrch · 2023-07-26T01:56:16Z

Test

[source-postgres, destination-postgres, source-fresh-desk (whoops)] ✅ (https://github.com/airbytehq/airbyte/actions/runs/5663728190)
source-freshdesk ✅

prepublish

source-postgres ✅
source-freshdesk ✅

Edit: everything passed, moving onto reverting source-faker changes

This reverts commit 26884cd.

alafanechere · 2023-07-26T10:41:25Z

@bnchrch I pushed a gitignore change to ignore the legacy pipeline reports paths. Otherwise a git add . will stage all the old report files a developer might have locally (like I did 😄 ).

alafanechere · 2023-07-26T11:03:37Z

@bnchrch I updated the poetry lock following the test dependency declaration.

bnchrch added 12 commits July 4, 2023 10:54

Move ci_connector_ops

d120804

Move ci_credentials

be05297

Move tools/ci_common_utils

0cc3654

Rename tools to airbyte-ci

cbadf69

Move to ci

e2f4984

Convert ci_credentials

cc96f19

Convert ci_common_utls

94e792e

Convert ci_connector_ops

3f83d32

Get pipelines running

dc60bdf

Move pipelines to own poetry project

22fd289

Update readme

ad13486

Delete

43a7916

bnchrch changed the title ~~WIP: Move tools/ci_* projects to airbyte-ci and update to use Poetry~~ Move tools/ci_* projects to airbyte-ci, update to use Poetry, bump to python 3.10 Jul 5, 2023

bnchrch requested a review from alafanechere July 5, 2023 06:44

bnchrch commented Jul 5, 2023

View reviewed changes

airbyte-ci/ci/ci_pipelines/README.md Outdated

Copy link

Contributor Author

bnchrch Jul 5, 2023

Choose a reason for hiding this comment

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

Pipelines/airbyte-ci are now their own package

bnchrch commented Jul 5, 2023

View reviewed changes

bnchrch added 3 commits July 5, 2023 16:26

Add ci_code_validator

79c47bf

Use pipx to install gha deps

2617d48

Fix'

9f7528f

bnchrch marked this pull request as ready for review July 7, 2023 22:46

bnchrch requested a review from a team July 7, 2023 22:46

bnchrch added 3 commits July 20, 2023 16:02

Merge remote-tracking branch 'origin/master' into bnchrch/ci/poetry-p…

45bbd69

…ipelines

Merge remote-tracking branch 'origin/master' into bnchrch/ci/poetry-p…

ecc588e

…ipelines

Ensure every thing is running

e4fca4c

bnchrch commented Jul 25, 2023

View reviewed changes

Add comment

4e18725

bnchrch requested review from evantahler, pedroslopez and erohmensing July 25, 2023 02:00

erohmensing approved these changes Jul 25, 2023

View reviewed changes

airbyte-ci/connectors/ci_code_validator/__init__.py Outdated

Copy link

Contributor

erohmensing Jul 25, 2023

Choose a reason for hiding this comment

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

I've never heard of this, if that helps anything!

alafanechere approved these changes Jul 25, 2023

View reviewed changes

bnchrch added 4 commits July 25, 2023 17:25

remove ci_code_validator

370c3fb

Address code review comments

7e92c19

Merge remote-tracking branch 'origin/master' into bnchrch/ci/poetry-p…

68758f9

…ipelines

add pipx install to acceptance-test-docker.sh

0edaf46

Run formater

4090391

bnchrch added 2 commits July 25, 2023 22:46

Revert "ceremonial source-faker change"

00e93dc

This reverts commit 26884cd.

Merge branch 'master' into bnchrch/ci/poetry-pipelines

5aa193a

octavia-squidington-iii removed the area/connectors Connector related issues label Jul 26, 2023

gitignore lecacy pipeline report path

261e289

update poetry.lock

4a0e143

skip upload if logs do not exist

ed1bc91

bnchrch enabled auto-merge (squash) July 26, 2023 15:28

bnchrch merged commit fb7258e into master Jul 26, 2023

bnchrch deleted the bnchrch/ci/poetry-pipelines branch July 26, 2023 15:50

		```

		This command installs `airbyte-ci` and makes it globally available in your terminal.

Move tools/ci_* projects to airbyte-ci, update to use Poetry, bump to python 3.10 #27957

Move tools/ci_* projects to airbyte-ci, update to use Poetry, bump to python 3.10 #27957

Conversation

bnchrch commented Jul 4, 2023 • edited Loading

What

Notes for reviewer

Left TODO

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alafanechere commented Jul 5, 2023

bnchrch commented Jul 5, 2023

alafanechere commented Jul 5, 2023

bnchrch commented Jul 5, 2023

octavia-squidington-iii commented Jul 25, 2023

source-faker test report (commit 26884cd0db) - ❌

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnchrch commented Jul 25, 2023

erohmensing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alafanechere left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

octavia-squidington-iii commented Jul 26, 2023

source-faker test report (commit 68758f99ba) - ❌

bnchrch commented Jul 26, 2023

octavia-squidington-iii commented Jul 26, 2023

source-faker test report (commit 4090391bac) - ❌

bnchrch commented Jul 26, 2023 • edited Loading

alafanechere commented Jul 26, 2023 • edited Loading

alafanechere commented Jul 26, 2023

bnchrch commented Jul 4, 2023 •

edited

Loading

source-faker test report (commit `26884cd0db`) - ❌

source-faker test report (commit `68758f99ba`) - ❌

source-faker test report (commit `4090391bac`) - ❌

bnchrch commented Jul 26, 2023 •

edited

Loading

alafanechere commented Jul 26, 2023 •

edited

Loading