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: improve QA checks for airbyte-enterprise #45393

Merged
merged 2 commits into from
Sep 11, 2024
Merged

airbyte-ci: improve QA checks for airbyte-enterprise #45393

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e416af2
airbyte-ci: improve QA checks for airbyte-enterprise
postamar Sep 11, 2024
60f160f
Update airbyte-ci/connectors/pipelines/README.md
postamar Sep 11, 2024
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
1 change: 1 addition & 0 deletions airbyte-ci/connectors/pipelines/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -843,6 +843,7 @@ airbyte-ci connectors --language=low-code migrate-to-manifest-only

| Version | PR | Description |
| ------- | ---------------------------------------------------------- |------------------------------------------------------------------------------------------------------------------------------|
| 4.35.3 | [#45393](https://github.com/airbytehq/airbyte/pull/45393) | Resolve symlinks in `SimpleDockerStep`. |
| 4.35.2 | [#45360](https://github.com/airbytehq/airbyte/pull/45360) | Updated dependencies. |
| 4.35.1 | [#45160](https://github.com/airbytehq/airbyte/pull/45160) | Remove deps.toml dependency for java connectors. |
| 4.35.0 | [#44879](https://github.com/airbytehq/airbyte/pull/44879) | Mount `components.py` when building manifest-only connector image |
Expand Down
24 changes: 16 additions & 8 deletions airbyte-ci/connectors/pipelines/pipelines/airbyte_ci/steps/docker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
#
# Copyright (c) 2023 Airbyte, Inc., all rights reserved.
#

from pathlib import Path
from typing import Dict, List, Optional

import dagger
Expand Down Expand Up @@ -55,13 +55,21 @@ def _mount_paths(self, container: dagger.Container) -> dagger.Container:
if path_to_mount.optional and not path_to_mount.get_path().exists():
continue

path_string = str(path_to_mount)
destination_path = f"/{path_string}"
if path_to_mount.is_file:
file_to_load = self.context.get_repo_file(path_string)
container = container.with_mounted_file(destination_path, file_to_load)
else:
container = container.with_mounted_directory(destination_path, self.context.get_repo_dir(path_string))
if path_to_mount.get_path().is_symlink():
container = self._mount_path(container, path_to_mount.get_path().readlink())

Copy link
Contributor

Choose a reason for hiding this comment

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

isn't an else missing there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, if it's not a symlink, then line 61 is enough

Copy link
Contributor

Choose a reason for hiding this comment

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

But line 61 will remount the path without resolution if the path is a symlink, you mean we want to mount both in any case right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

L61 mounts the symlink itself. It's one of the quirks of buildkit that it doesn't resolve the symlink before mounting it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we're not the only people to be annoyed by this: moby/moby#1676

container = self._mount_path(container, path_to_mount.get_path())
return container

def _mount_path(self, container: dagger.Container, path: Path) -> dagger.Container:
path_string = str(path)
destination_path = f"/{path_string}"
if path.is_file():
file_to_load = self.context.get_repo_file(path_string)
container = container.with_mounted_file(destination_path, file_to_load)
else:
dir_to_load = self.context.get_repo_dir(path_string)
container = container.with_mounted_directory(destination_path, dir_to_load)
return container

async def _install_internal_tools(self, container: dagger.Container) -> dagger.Container:
Expand Down
4 changes: 0 additions & 4 deletions airbyte-ci/connectors/pipelines/pipelines/models/steps.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,10 +62,6 @@ def __post_init__(self) -> None:
def __str__(self) -> str:
return str(self.path)

@property
def is_file(self) -> bool:
return self.get_path().is_file()


@dataclass(kw_only=True, frozen=True)
class Result:
Expand Down
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 = "4.35.2"
version = "4.35.3"
description = "Packaged maintained by the connector operations team to perform CI for connectors' pipelines"
authors = ["Airbyte <contact@airbyte.io>"]

Expand Down
Loading