-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Airbyte-ci: Generate binary files for airbyte-ci #31930
Changes from 37 commits
2865e12
80d8514
56c272a
d68aca5
b3e941f
d382026
a20e9e5
cea4b2a
9254e6d
964a1ca
010470f
7b2664b
d64044b
2ab5b91
072b27a
043a7a6
726f287
9f12faa
248d584
f97ea25
77d6f47
1a4339b
0686c9e
f7dbb86
5accdc0
ab427b2
32c586b
0acb859
a45b667
5e79294
be26ee9
29edc93
0f89db3
ff9302e
ad101d5
3325528
90e666e
325fd2b
9863821
1ad6891
b4674d5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,7 +30,6 @@ MANIFEST | |
# Usually these files are written by a python script from a template | ||
# before PyInstaller builds the exe, so as to inject date/other infos into it. | ||
*.manifest | ||
*.spec | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you share why it's not gitignored anymore? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, this was an artifact from attempting to use spec files as the way to define pyinstaller configuration. will revert |
||
|
||
# Installer logs | ||
pip-log.txt | ||
|
bnchrch marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we explain airbyte-ci developers how to build the binary locally in this file? |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -123,7 +123,7 @@ At this point you can run `airbyte-ci` commands. | |
|
||
| Option | Default value | Mapped environment variable | Description | | ||
| --------------------------------------- | ------------------------------- | ----------------------------- | ------------------------------------------------------------------------------------------- | | ||
| `--no-tui` | | | Disables the Dagger terminal UI. | | ||
| `--enable-dagger-run/--disable-dagger-run` | `--enable-dagger-run`` | | Disables the Dagger terminal UI. | | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice! I was worried about merging airbyte CI and airbyte CI internal but this works |
||
| `--is-local/--is-ci` | `--is-local` | | Determines the environment in which the CLI runs: local environment or CI environment. | | ||
| `--git-branch` | The checked out git branch name | `CI_GIT_BRANCH` | The git branch on which the pipelines will run. | | ||
| `--git-revision` | The current branch head | `CI_GIT_REVISION` | The commit hash on which the pipelines will run. | | ||
|
@@ -408,6 +408,7 @@ This command runs the Python tests for a airbyte-ci poetry package. | |
## Changelog | ||
| Version | PR | Description | | ||
| ------- | ---------------------------------------------------------- | --------------------------------------------------------------------------------------------------------- | | ||
| 2.6.0 | [#31930](https://github.com/airbytehq/airbyte/pull/31930) | Merge airbyte-ci-internal into airbyte-ci | | ||
| 2.5.8 | [#32402](https://github.com/airbytehq/airbyte/pull/32402) | Set Dagger Cloud token for airbyters only | | ||
| 2.5.7 | [#31628](https://github.com/airbytehq/airbyte/pull/31628) | Add ClickPipelineContext class | | ||
| 2.5.6 | [#32139](https://github.com/airbytehq/airbyte/pull/32139) | Test coverage report on Python connector UnitTest. | | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ | |
import logging | ||
import multiprocessing | ||
import os | ||
import sys | ||
from pathlib import Path | ||
from typing import List, Optional | ||
|
||
|
@@ -19,7 +20,7 @@ | |
from pipelines.cli.click_decorators import click_append_to_context_object, click_ignore_unused_kwargs, click_merge_args_into_context_obj | ||
from pipelines.cli.lazy_group import LazyGroup | ||
from pipelines.cli.telemetry import click_track_command | ||
from pipelines.consts import LOCAL_PIPELINE_PACKAGE_PATH, CIContext | ||
from pipelines.consts import DAGGER_WRAP_ENV_VAR_NAME, LOCAL_PIPELINE_PACKAGE_PATH, CIContext | ||
from pipelines.helpers import github | ||
from pipelines.helpers.git import ( | ||
get_current_git_branch, | ||
|
@@ -56,7 +57,7 @@ def display_welcome_message() -> None: | |
) | ||
|
||
|
||
def check_up_to_date() -> bool: | ||
def check_up_to_date(throw_as_error=False) -> bool: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: |
||
"""Check if the installed version of pipelines is up to date.""" | ||
latest_version = get_latest_version() | ||
if latest_version != __installed_version__: | ||
|
@@ -68,7 +69,12 @@ def check_up_to_date() -> bool: | |
|
||
🚨🚨🚨🚨🚨🚨🚨🚨🚨🚨🚨🚨🚨🚨🚨🚨🚨🚨🚨🚨🚨🚨🚨🚨🚨🚨🚨🚨🚨🚨🚨 | ||
""" | ||
raise Exception(upgrade_error_message) | ||
|
||
if throw_as_error: | ||
raise Exception(upgrade_error_message) | ||
else: | ||
logging.warning(upgrade_error_message) | ||
return False | ||
|
||
main_logger.info(f"pipelines is up to date. Installed version: {__installed_version__}. Latest version: {latest_version}") | ||
return True | ||
|
@@ -206,6 +212,38 @@ def check_local_docker_configuration(): | |
) | ||
|
||
|
||
def is_dagger_run_enabled_by_default() -> bool: | ||
dagger_run_by_default = [ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we store it in a constant variable at module level? |
||
["connectors", "test"], | ||
["connectors", "build"], | ||
["test"], | ||
["metadata_service"], | ||
] | ||
erohmensing marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
for command_tokens in dagger_run_by_default: | ||
if all(token in sys.argv for token in command_tokens): | ||
return True | ||
|
||
return False | ||
|
||
|
||
def check_dagger_wrap(): | ||
""" | ||
Check if the command is already wrapped by dagger run. | ||
This is useful to avoid infinite recursion when calling dagger run from dagger run. | ||
""" | ||
return os.getenv(DAGGER_WRAP_ENV_VAR_NAME) == "true" | ||
|
||
|
||
def is_current_process_wrapped_by_dagger_run() -> bool: | ||
""" | ||
Check if the current process is wrapped by dagger run. | ||
""" | ||
called_with_dagger_run = check_dagger_wrap() | ||
main_logger.info(f"Called with dagger run: {called_with_dagger_run}") | ||
return called_with_dagger_run | ||
|
||
|
||
async def get_modified_files_str(ctx: click.Context): | ||
modified_files = await get_modified_files( | ||
ctx.obj["git_branch"], | ||
|
@@ -231,6 +269,7 @@ async def get_modified_files_str(ctx: click.Context): | |
}, | ||
) | ||
@click.version_option(__installed_version__) | ||
@click.option("--enable-dagger-run/--disable-dagger-run", default=is_dagger_run_enabled_by_default) | ||
@click.option("--is-local/--is-ci", default=True) | ||
@click.option("--git-branch", default=get_current_git_branch, envvar="CI_GIT_BRANCH") | ||
@click.option("--git-revision", default=get_current_git_revision, envvar="CI_GIT_REVISION") | ||
|
@@ -247,6 +286,7 @@ async def get_modified_files_str(ctx: click.Context): | |
@click.option("--ci-git-user", default="octavia-squidington-iii", envvar="CI_GIT_USER", type=str) | ||
@click.option("--ci-github-access-token", envvar="CI_GITHUB_ACCESS_TOKEN", type=str) | ||
@click.option("--ci-report-bucket-name", envvar="CI_REPORT_BUCKET_NAME", type=str) | ||
@click.option("--ci-artifact-bucket-name", envvar="CI_ARTIFACT_BUCKET_NAME", type=str) | ||
@click.option( | ||
"--ci-gcs-credentials", | ||
help="The service account to use during CI.", | ||
|
@@ -268,12 +308,20 @@ async def get_modified_files_str(ctx: click.Context): | |
@click_ignore_unused_kwargs | ||
async def airbyte_ci(ctx: click.Context): # noqa D103 | ||
display_welcome_message() | ||
|
||
if ctx.obj["enable_dagger_run"] and not is_current_process_wrapped_by_dagger_run(): | ||
main_logger.debug("Re-Running airbyte-ci with dagger run.") | ||
from pipelines.cli.dagger_run import call_current_command_with_dagger_run | ||
|
||
call_current_command_with_dagger_run() | ||
return | ||
|
||
if ctx.obj["is_local"]: | ||
# This check is meaningful only when running locally | ||
# In our CI the docker host used by the Dagger Engine is different from the one used by the runner. | ||
check_local_docker_configuration() | ||
|
||
check_up_to_date() | ||
check_up_to_date(throw_as_error=ctx.obj["is_local"]) | ||
|
||
if not ctx.obj["is_local"]: | ||
log_git_info(ctx) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,7 +14,7 @@ | |
|
||
import pkg_resources | ||
import requests | ||
from pipelines.cli.airbyte_ci import set_working_directory_to_root | ||
from pipelines.consts import DAGGER_WRAP_ENV_VAR_NAME | ||
|
||
LOGGER = logging.getLogger(__name__) | ||
BIN_DIR = Path.home() / "bin" | ||
|
@@ -72,6 +72,10 @@ def get_dagger_cli_version(dagger_path: Optional[str]) -> Optional[str]: | |
|
||
|
||
def check_dagger_cli_install() -> str: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: shall we rename this to
|
||
""" | ||
If the dagger CLI is not installed, install it. | ||
""" | ||
|
||
expected_dagger_cli_version = get_current_dagger_sdk_version() | ||
dagger_path = get_dagger_path() | ||
if dagger_path is None: | ||
|
@@ -89,17 +93,21 @@ def check_dagger_cli_install() -> str: | |
return dagger_path | ||
|
||
|
||
def main(): | ||
set_working_directory_to_root() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. woohoo, no longer doing this in 2 places! |
||
def mark_dagger_wrap(): | ||
""" | ||
Mark that the dagger wrap has been applied. | ||
""" | ||
os.environ[DAGGER_WRAP_ENV_VAR_NAME] = "true" | ||
|
||
|
||
def call_current_command_with_dagger_run(): | ||
mark_dagger_wrap() | ||
if (os.environ.get("AIRBYTE_ROLE") == "airbyter") or (os.environ.get("CI") == "True"): | ||
os.environ[DAGGER_CLOUD_TOKEN_ENV_VAR_NAME_VALUE[0]] = DAGGER_CLOUD_TOKEN_ENV_VAR_NAME_VALUE[1] | ||
|
||
exit_code = 0 | ||
if len(sys.argv) > 1 and any([arg in ARGS_DISABLING_TUI for arg in sys.argv]): | ||
command = ["airbyte-ci-internal"] + [arg for arg in sys.argv[1:] if arg != "--no-tui"] | ||
else: | ||
dagger_path = check_dagger_cli_install() | ||
command = [dagger_path, "run", "airbyte-ci-internal"] + sys.argv[1:] | ||
dagger_path = check_dagger_cli_install() | ||
command = [dagger_path, "run"] + sys.argv | ||
Comment on lines
+103
to
+110
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I think this change breaks the feature of running cd airbyte-ci/connectors/pipelines
./dist/airbyte-ci connectors --name=source-faker test Output: INFO pipelines.cli.dagger_run: Running command: ['/Users/augustin/bin/dagger', 'run', './dist/airbyte-ci', 'connectors', '--name=source-faker', 'test'] dagger_run.py:113
█ [0.00s] ERROR ./dist/airbyte-ci connectors --name=source-faker test This is not happening after I added |
||
try: | ||
try: | ||
LOGGER.info(f"Running command: {command}") | ||
|
@@ -110,7 +118,3 @@ def main(): | |
except subprocess.CalledProcessError as e: | ||
exit_code = e.returncode | ||
sys.exit(exit_code) | ||
|
||
|
||
if __name__ == "__main__": | ||
main() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make the destination bucket / path a workflow input?
Would be nice to be able to write to safely write to
pre-releases
to make sure an upload from a PR without a bumped airbyte-ci version never overwrites a previous artifact. An alternative way to avoid accidental overwrite would be to add a commit id postfix.