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.

Sign up for GitHub

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

Jump to bottom

airbyte-ci: ClickPipelineContext handles dagger logging #33419

Merged
merged 1 commit into from
Dec 15, 2023
Merged

airbyte-ci: ClickPipelineContext handles dagger logging #33419

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion airbyte-ci/connectors/pipelines/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -502,7 +502,8 @@ This command runs the Python tests for a airbyte-ci poetry package.

| Version | PR | Description |
| ------- | ---------------------------------------------------------- | --------------------------------------------------------------------------------------------------------- |
| 2.10.11 | [#33497](https://github.com/airbytehq/airbyte/pull/33497) | Consider nested .gitignore rules in format. |
| 2.10.12 | [#33419](https://github.com/airbytehq/airbyte/pull/33419) | Make ClickPipelineContext handle dagger logging. |
| 2.10.11 | [#33497](https://github.com/airbytehq/airbyte/pull/33497) | Consider nested .gitignore rules in format. |
| 2.10.10 | [#33449](https://github.com/airbytehq/airbyte/pull/33449) | Add generated metadata models to the default format ignore list. |
| 2.10.9 | [#33370](https://github.com/airbytehq/airbyte/pull/33370) | Fix bug that broke airbyte-ci test |
| 2.10.8 | [#33249](https://github.com/airbytehq/airbyte/pull/33249) | Exclude git ignored files from formatting. |
Expand Down
10 changes: 1 addition & 9 deletions airbyte-ci/connectors/pipelines/pipelines/airbyte_ci/format/format_command.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,16 +121,8 @@ async def invoke(self, ctx: click.Context, click_pipeline_context: ClickPipeline
Returns:
Any: The result of running the command
"""
dagger_logs_file_descriptor, dagger_logs_temp_file_path = tempfile.mkstemp(
dir="/tmp", prefix=f"format_{self.formatter.value}_dagger_logs_", suffix=".log"
)
# Create a FileIO object from the file descriptor
dagger_logs = io.FileIO(dagger_logs_file_descriptor, "w+")
self.logger.info(f"Running {self.formatter.value} formatter. Logging dagger output to {dagger_logs_temp_file_path}")

dagger_client = await click_pipeline_context.get_dagger_client(
pipeline_name=f"Format {self.formatter.value}", log_output=dagger_logs
)
dagger_client = await click_pipeline_context.get_dagger_client(pipeline_name=f"Format {self.formatter.value}")
dir_to_format = self.get_dir_to_format(dagger_client)

container = self.get_format_container_fn(dagger_client, dir_to_format)
Expand Down
26 changes: 23 additions & 3 deletions airbyte-ci/connectors/pipelines/pipelines/models/contexts/click_pipeline_context.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,16 @@
# Copyright (c) 2023 Airbyte, Inc., all rights reserved.
#

import io
import sys
from typing import Any, Callable, Optional, TextIO
import tempfile
from typing import Any, Callable, Optional, TextIO, Tuple

import anyio
import dagger
from asyncclick import Context, get_current_context
from dagger.api.gen import Client, Container
from pipelines import main_logger
from pipelines.cli.click_decorators import LazyPassDecorator
from pydantic import BaseModel, Field, PrivateAttr

Expand Down Expand Up @@ -76,15 +79,15 @@ def __init__(self, **data: dict[str, Any]):

_dagger_client_lock: anyio.Lock = PrivateAttr(default_factory=anyio.Lock)

async def get_dagger_client(self, pipeline_name: Optional[str] = None, log_output: Optional[TextIO] = sys.stdout) -> Client:
async def get_dagger_client(self, pipeline_name: Optional[str] = None) -> Client:
"""
Get (or initialize) the Dagger Client instance.
"""
if not self._dagger_client:
async with self._dagger_client_lock:
if not self._dagger_client:
connection = dagger.Connection(dagger.Config(log_output=log_output))

connection = dagger.Connection(dagger.Config(log_output=self.get_log_output()))
"""
Sets up the '_dagger_client' attribute, intended for single-threaded use within connectors.

Expand All @@ -97,6 +100,23 @@ async def get_dagger_client(self, pipeline_name: Optional[str] = None, log_outpu
assert self._dagger_client, "Error initializing Dagger client"
return self._dagger_client.pipeline(pipeline_name) if pipeline_name else self._dagger_client

def get_log_output(self) -> TextIO:
# This `show_dagger_logs` flag is likely going to be removed in the future.
# See https://github.com/airbytehq/airbyte/issues/33487
if self.params.get("show_dagger_logs", False):
return sys.stdout
else:
log_output, self._click_context().obj["dagger_logs_path"] = self._create_dagger_client_log_file()
return log_output

def _create_dagger_client_log_file(self) -> Tuple[io.FileIO, str]:
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ Instead of adding this logic here, could we not hoist it into airbyte_ci.py or dagger_run.py and make it shared between all pipelines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's something I considered but as the log file is tightly coupled to the client I prefer to keep it where the client is instantiated.
I'm also conscious that we currently have inconsistencies in the way we handle dagger client logging in our commands.
I created this issue to consolidate the approach.
TLDR; all our commands should get their dagger client from ClickPipelineContext

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds fine then!

At that point in time we should be in a good place to introduce the GlobalSettings pattern from aircmd,

"""
Create the dagger client log file.
"""
dagger_logs_file_descriptor, dagger_logs_temp_file_path = tempfile.mkstemp(dir="/tmp", prefix=f"dagger_client_", suffix=".log")
main_logger.info(f"Dagger client logs stored in {dagger_logs_temp_file_path}")
return io.FileIO(dagger_logs_file_descriptor, "w+"), dagger_logs_temp_file_path


# Create @pass_pipeline_context decorator for use in click commands
pass_pipeline_context: LazyPassDecorator = LazyPassDecorator(ClickPipelineContext)
2 changes: 1 addition & 1 deletion airbyte-ci/connectors/pipelines/pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ build-backend = "poetry.core.masonry.api"

[tool.poetry]
name = "pipelines"
version = "2.10.11"
version = "2.10.12"
description = "Packaged maintained by the connector operations team to perform CI for connectors' pipelines"
authors = ["Airbyte <contact@airbyte.io>"]

Expand Down
2 changes: 2 additions & 0 deletions airbyte-ci/connectors/pipelines/tests/test_models/test_click_pipeline_context.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ def cli():
pass

ctx = click.Context(cli)
ctx.obj = {"foo": "bar"}
ctx.params = {"baz": "qux"}

async with ctx.scope():
click_pipeline_context = ClickPipelineContext()
Expand Down
Loading