Airbyte-Ci: Add makefile for new tool installation #32484

bnchrch · 2023-11-14T06:06:00Z

Overview

Updates the python installed entry point to be airbyte-ci-dev
Adds auto upgrading to airbyte-ci
Adds a makefile that can
1. Install airbyte-ci from binary
2. install airbyte-ci-dev from pipx
3. check if your path is incorrect

vercel · 2023-11-14T06:06:06Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
airbyte-docs	⬜️ Ignored (Inspect)	Visit Preview		Nov 29, 2023 0:23am

bnchrch · 2023-11-16T09:22:09Z

airbyte-ci/connectors/pipelines/pyproject.toml

@@ -40,6 +40,7 @@ poethepoet = "^0.24.2"

 [tool.poetry.scripts]
 airbyte-ci = "pipelines.cli.airbyte_ci:airbyte_ci"


I left this in for now as I have a hope that the auto upgrade feature will install the binary automatically for people

Makefile

alafanechere

make tools.airbyte-ci.install worked for me!
I made a couple of suggestions, let me know what you think.
My main one is:

Now that we have a binary can we discard the use of pipx and encourage airbyte-ci developers to iterate using poetry run airbyte-ci?

Makefile

alafanechere · 2023-11-16T10:31:09Z

Makefile

+
+tools.install: tools.airbyte-ci.install tools.check.airbyte-ci
+
+.PHONY: tools.install tools.airbyte-ci.install tools.airbyte-ci-dev.install


What's the purpose of .PHONY?

I brought it over from the makefile in the cloud repo.

I loosely understood it as a way to declare what was a command

Looks like that was only partially right.

Heres chatgpts explanation

airbyte-ci/connectors/pipelines/README.md

alafanechere · 2023-11-16T10:35:12Z

Makefile

+		echo "airbyte-ci is either not on the PATH or not pointing to $$EXPECTED_PATH"; \
+		echo "Check that airbyte-ci exists at $$HOME/.local/bin and $$HOME/.local/bin is part the PATH"; \
+		echo "If it does try running removing all instances of airbyte-ci on your path, then run `make tools.install` again"; \
+		exit 1; \


Can we be more explicit by:

explaining how to remove the pipx previous install from the path

explaining how to add $$HOME/.local/bin to the path

I can do "explaining how to remove the pipx previous install from the path" for sure by pointing to using a clean method. Explaining how to add to a path may be better left for the user to google

By default .local should be part of the path though. So hopefully we dont run into this

alafanechere · 2023-11-16T10:46:04Z

Makefile

+tools.airbyte-ci-dev.install: ## Install the development version of airbyte-ci
+	##@ Install airbyte-ci development version
+	pipx install --editable --force --python=python3.10 airbyte-ci/connectors/pipelines/; \
+	echo "Development version of airbyte-ci installed.";


Shall we discard the dev mode and encourage airbyte-ci developers to use poetry run airbyte-ci ?

The thing is I think they both have their place?

poetry is useful for intense development where you are updating dependencys (particularly local ones)

pipx is useful if you want to install the dev version of the tool and use it in other areas of the code base (where you may need a different virtual env)

Im thinking in the makefile we use pipx for dev as its likely the entry point for "A non connector ops dev looking to test out a airbyte-ci change in their area of the code base"

Then in the setup readme for airbyte-ci push people to use poetry as thats the likely entry point for "A connector ops dev looking to make large updates to airbyte-ci"

wdyt?

Makes sense, I have the impression that A non connector ops dev looking to test out a airbyte-ci change in their area of the code base is not common ATM, but let's not block this use case 👍

alafanechere · 2023-11-16T10:50:43Z

Makefile

+	fi; \
+	echo "airbyte-ci is correctly installed at $$EXPECTED_PATH"
+
+


Shall we add a clean / uninstall command called before install that would:

run pipx uninstall pipelines

delete any existing airbyte-ci in the path?

Im team clean as a standalone step!

but we probably dont want it before install because

It would be a time penalty

In the binary future it would be unessesary as pipx should have gone away for all but c-ops devs

airbyte-ci/connectors/pipelines/pipelines/cli/airbyte_ci.py

Makefile

bnchrch · 2023-11-16T19:32:34Z

@alafanechere This is back and ready for review

Commands have been moved to shellscript
Echo is working properly as a result
Add make tools.airbyte-ci.clean which cleans up the environment
Updated docs

I also left 2 follow ups for you on

Gradle install
Using pipx

that I would love your thoughts on

postamar

Just a few comments but nothing major.

postamar · 2023-11-17T02:44:20Z

tools/bin/make/airbyte_ci_check.sh

@@ -0,0 +1,30 @@
+#!/bin/bash


I don't know why but I was told that #!/usr/bin/env bash is preferable.

Yup good call out

postamar · 2023-11-17T02:45:17Z

tools/bin/make/airbyte_ci_check.sh

@@ -0,0 +1,30 @@
+#!/bin/bash
+


set -o errexit -o nounset -o pipefail in all bash scripts.

Consider running these through the shellcheck linter.

postamar · 2023-11-17T02:48:35Z

tools/bin/make/airbyte_ci_dev_install.sh

+# Check if pyenv is installed, and install it if not
+if ! which pyenv >/dev/null; then
+    echo "pyenv not found, installing pyenv..."
+    brew install pyenv


I'd rather you fail with an error message telling the user to brew install pyenv. Perhaps that's just me.

postamar · 2023-11-17T02:50:57Z

tools/bin/make/airbyte_ci_install.sh

+mkdir -p ~/.local/bin
+
+# Remove the old binary if it exists
+rm -f ~/.local/bin/airbyte-ci


Consider curling the new binary first, before removing the old one. The download may fail and then the user is up the proverbial creek.

Yeah fair. I got lazy and ignored doing a tmp folder

If you create a temp folder then you also need to add an exit trap to clean it up and those are easy to not get right. Consider doing a curl -o instead.

I did have -o set. but it was failing to overwrite and kept leaving the dev sym link.

Ill look into it a bit more

Did some research, the main pain is that curls download will follow the symbolic link.

To get it not to, or to delete the symbolic link with a fallback that recreates it is a bit of fiddly code
https://superuser.com/questions/303559/replace-symbolic-links-with-files

Given that once the binary is widely used we will be removing the airbyte-ci python entry point in favour of airbyte-ci-dev this symlink issue should go away.

so for not I opted to only delete the destination path when its a symlink.

Worst case scenario they run clean and install again

postamar · 2023-11-17T02:52:28Z

Makefile

+tools.install: tools.airbyte-ci.install tools.airbyte-ci.check
+
+.PHONY: tools.install tools.airbyte-ci.install tools.airbyte-ci-dev.install tools.airbyte-ci.check tools.airbyte-ci.clean
+


Do we want this makefile in the root? Seems like it should be in airbyte-ci? Just a thought.

Good callout. Im thinking that this makefile can become an entry point for setting up other tools. Like precommit hooks

alafanechere

Nice stuff here! I tried the flow of clean / dev install / normal install and it worked 👍
I added minor suggestions.
About the gradle install I think we should do it (here or in a followup PR), because otherwise java developers will always use the "dev version" that we might don't want to release... It can make it harder to reproduce their problems. Feel free to do it in a separate PR.

airbyte-ci/connectors/pipelines/README.md

alafanechere · 2023-11-17T08:32:25Z

tools/bin/make/airbyte_ci_dev_install.sh

+# Check if pyenv is installed, and install it if not
+if ! which pyenv >/dev/null; then
+    echo "pyenv not found, installing pyenv..."
+    brew install pyenv


alafanechere · 2023-11-17T08:37:21Z

tools/bin/make/airbyte_ci_dev_install.sh

+# Check if pyenv is installed, and install it if not
+if ! which pyenv >/dev/null; then
+    echo "pyenv not found, installing pyenv..."
+    brew install pyenv
+    echo "pyenv installed."
+else
+    echo "pyenv is already installed."
+fi


I'm not sure pyenv is a requirement here... I think the requirement is that the current Python version is 3.10, and pyenv is a tool to switch Python version. pipx install --editable --force --python=python3.10 airbyte-ci/connectors/pipelines/ will fail if the user is not on python3.10.

My suggestion is to make this script only check that the output of python --version matches Python 3.10.* . In case of error we can suggest users to:

brew install pyenv

pyenv install 3.10.12

pyenv shell 3.10.12

But we actually don't care if users use pyenv or any other tool to install python 3.10.

Moreover, if executed from the repo root, pyenv won't detect that the version we want to use is 3.10 as its defined in airbyte-ci/.python-version

Good call out, also worth specificying we do have a .python-version file in the root of the repo set to 3.10
https://github.com/airbytehq/airbyte/blob/89f6ecc0d0244cb24514f6011aa0d84a9b43f05b/.python-version#L0-L1

alafanechere · 2023-11-17T08:43:40Z

tools/bin/make/airbyte_ci_install.sh

Shall we keep these scripts under airbyte-ci/connectors/pipelines? They're not coupled with make anymore

Good call, lets move them

…ci-binary-locally

alafanechere

I reviewed the auto_uppdate.py.
I'm concerned by the fact that it strongly couples airbyte-ci install/upgrade to our repo.
We can change this in follow up PRs when we'll try to make airbyte-ci run outside of airbyte repo, but I don't think it's not a heavy lift to take the less coupled approach I suggest.

airbyte-ci/connectors/pipelines/pipelines/cli/auto_update.py

alafanechere · 2023-11-20T08:46:28Z

airbyte-ci/connectors/pipelines/pipelines/cli/auto_update.py

+BINARY_UPGRADE_COMMAND = "make tools.airbyte-ci.install"
+DEV_UPGRADE_COMMAND = "make tools.airbyte-ci-dev.install"


Can we make these upgrade command download and execute the shell scripts from https://raw.github.com? The upgrade would otherwise be tied to the existence of the repo.

You know what yes, me wanting to tie it to the makefile was wrong! Will change

Co-authored-by: Augustin <augustin@airbyte.io>

bnchrch · 2023-11-24T00:42:17Z

@alafanechere I took your usable outside the repo to heart and moved these install scripts from bash to python and exposed them with airbyte-ci update so now the airbyte-ci will always package its install scripts and reference them directly.

…ci-binary-locally

alafanechere · 2023-11-28T11:00:18Z

Makefile

+##@ Define the default version
+VERSION ?= latest


Suggested change

##@ Define the default version

VERSION ?= latest

##@ Define the default airbyte-ci version

AIRBYTE_CI_VERSION ?= latest

As this is the root Makefile I think its better to mention that the version is the airbyte-ci one.

alafanechere · 2023-11-28T11:19:12Z

airbyte-ci/connectors/pipelines/pipelines/external_scripts/airbyte_ci_install.py

+    """
+    OS = os.uname().sysname
+    if OS == "Linux":
+        print(f"Linux based system detected.")


I suggest using rich for nicer console output. https://github.com/Textualize/rich

I would love to! But thats the downside of this approach. We cannot add an thirdparty dependencies to these files.

If we do then we need the user to run a pip install before they run the script.

This is why we have this disclaimer

airbyte/airbyte-ci/connectors/pipelines/pipelines/external_scripts/airbyte_ci_install.py

Line 4 in 4fca343

# Meaning, no external dependencies are allowed as we don't want users to have to run anything

alafanechere · 2023-11-28T11:20:07Z

airbyte-ci/connectors/pipelines/pipelines/external_scripts/airbyte_ci_install.py

+        os.chmod(destination_path, 0o755)
+
+    # ASCII Art and Completion Message
+    install_complete_message = f"""


You can go fancy here with rich-pixels https://pypi.org/project/rich-pixels/

alafanechere · 2023-11-28T11:22:19Z

@alafanechere I took your usable outside the repo to heart and moved these install scripts from bash to python and exposed them with airbyte-ci update so now the airbyte-ci will always package its install scripts and reference them directly.

Super cool! Shall we expose a tools.airbyte-ci.updatetarget to make sure that the update story always use the same airbyte-ci wrapped logic?

alafanechere · 2023-11-28T11:25:49Z

Current dependencies on/for this PR:

master
- PR Airbyte-Ci: Add makefile for new tool installation #32484 👈
  - PR airbyte-ci: run format fix all in a pre-push hook #32865

This stack of pull requests is managed by Graphite.

…ci-binary-locally

bnchrch force-pushed the bnchrch/ci/install-ci-binary-locally branch from 62bdcb4 to 3da4c81 Compare November 16, 2023 02:19

bnchrch marked this pull request as ready for review November 16, 2023 09:20

bnchrch requested review from a team and alafanechere November 16, 2023 09:21

bnchrch commented Nov 16, 2023

View reviewed changes

Makefile Outdated Show resolved Hide resolved

alafanechere requested changes Nov 16, 2023

View reviewed changes

alafanechere force-pushed the bnchrch/ci/install-ci-binary-locally branch 2 times, most recently from 9a6dae7 to d806391 Compare November 16, 2023 11:38

bnchrch added 8 commits November 16, 2023 11:29

Add makefile skeleton

8f54ca3

Move to dev

a489b9e

Add autoupdate

08c760f

Update docs

c1f252b

Move to shell scripts

879610b

Add clean command

e247a0c

Update readme and warning

b0825ed

Address code review comments

9e4dcf6

bnchrch force-pushed the bnchrch/ci/install-ci-binary-locally branch from d806391 to 9e4dcf6 Compare November 16, 2023 19:29

bnchrch requested review from alafanechere and a team November 16, 2023 19:29

Add comment

a4713ef

Automated Commit - Formatting Changes

8853b9b

octavia-squidington-iii added area/connectors Connector related issues connectors/source/bing-ads labels Nov 16, 2023

octavia-squidington-iv requested a review from a team November 16, 2023 19:35

postamar reviewed Nov 17, 2023

View reviewed changes

alafanechere approved these changes Nov 17, 2023

View reviewed changes

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

b781969

…ci-binary-locally

bnchrch added 3 commits November 17, 2023 11:25

Update bash headers

22f5408

Shellcheck

f42c681

Move install scripts to airbyte-ci

9f7df97

octavia-squidington-iii removed the area/connectors Connector related issues label Nov 17, 2023

bnchrch added 2 commits November 17, 2023 13:12

Move to auto_update.py

6a43a05

Format

e2a96d6

alafanechere reviewed Nov 20, 2023

View reviewed changes

bnchrch and others added 5 commits November 23, 2023 14:08

Apply suggestions from code review

d7b2962

Co-authored-by: Augustin <augustin@airbyte.io>

Move scripts to pipelines

1859df4

Bump version

4ae088f

Update readme

895fd0d

Do not check for update in CI

fe1e809

bnchrch added 2 commits November 24, 2023 14:50

Add log

ff83907

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

8eb44a1

…ci-binary-locally

vercel bot deployed to Preview November 27, 2023 17:36 View deployment

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

029ad64

…ci-binary-locally

vercel bot deployed to Preview November 28, 2023 01:47 View deployment

alafanechere reviewed Nov 28, 2023

View reviewed changes

alafanechere mentioned this pull request Nov 28, 2023

airbyte-ci: run format fix all in a pre-push hook #32865

Closed

bnchrch added 3 commits November 28, 2023 16:18

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

b0529f7

…ci-binary-locally

Makefile update

b2f0f22

Update disclaimer

4fca343

bnchrch merged commit 4b69947 into master Nov 29, 2023
22 checks passed

bnchrch deleted the bnchrch/ci/install-ci-binary-locally branch November 29, 2023 01:57

		@@ -40,6 +40,7 @@ poethepoet = "^0.24.2"

		[tool.poetry.scripts]
		airbyte-ci = "pipelines.cli.airbyte_ci:airbyte_ci"


		tools.install: tools.airbyte-ci.install tools.check.airbyte-ci

		.PHONY: tools.install tools.airbyte-ci.install tools.airbyte-ci-dev.install

		fi; \
		echo "airbyte-ci is correctly installed at $$EXPECTED_PATH"

		BINARY_UPGRADE_COMMAND = "make tools.airbyte-ci.install"
		DEV_UPGRADE_COMMAND = "make tools.airbyte-ci-dev.install"

Airbyte-Ci: Add makefile for new tool installation #32484

Airbyte-Ci: Add makefile for new tool installation #32484

Conversation

bnchrch commented Nov 14, 2023 • edited Loading

Overview

vercel bot commented Nov 14, 2023 • edited Loading

Choose a reason for hiding this comment

alafanechere left a comment • edited Loading

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

bnchrch commented Nov 16, 2023 • edited Loading

postamar 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

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

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

bnchrch commented Nov 24, 2023

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 Nov 28, 2023

alafanechere commented Nov 28, 2023

bnchrch commented Nov 14, 2023 •

edited

Loading

vercel bot commented Nov 14, 2023 •

edited

Loading

alafanechere left a comment •

edited

Loading

bnchrch commented Nov 16, 2023 •

edited

Loading