Skip to content

airbyte-ci: fix format cli usage in airbyte-enterprise #43386

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

Merged
merged 3 commits into from
Aug 8, 2024
Merged

airbyte-ci: fix format cli usage in airbyte-enterprise #43386

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6847b9e
airbyte-ci: fix format cli usage in airbyte-enterprise
Aug 8, 2024
b44efb4
airbyte-ci: add missing mvn flags in format
Aug 8, 2024
84e6a98
airbyte-ci: bump version and update changelog
Aug 8, 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 @@ -789,6 +789,7 @@ airbyte-ci connectors --language=low-code migrate-to-manifest-only

| Version | PR | Description |
|---------| ---------------------------------------------------------- |------------------------------------------------------------------------------------------------------------------------------|
| 4.30.1 | [#43386](https://github.com/airbytehq/airbyte/pull/43386) | Fix 'format' command usage bug in airbyte-enterprise. |
| 4.30.0 | [#42583](https://github.com/airbytehq/airbyte/pull/42583) | Updated dependencies |
| 4.29.0 | [#42576](https://github.com/airbytehq/airbyte/pull/42576) | New command: `migrate-to-manifest-only` |
| 4.28.3 | [#42046](https://github.com/airbytehq/airbyte/pull/42046) | Trigger connector tests on doc change. |
Expand Down
2 changes: 1 addition & 1 deletion airbyte-ci/connectors/pipelines/pipelines/airbyte_ci/format/configuration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ class FormatConfiguration:
Formatter.JAVA,
["**/*.java", "**/*.kt", "**/*.gradle"],
format_java_container,
["mvn -f spotless-maven-pom.xml spotless:apply clean"],
["mvn -f spotless-maven-pom.xml --errors --batch-mode spotless:apply clean"],
),
# Run prettier on all json and yaml files.
FormatConfiguration(
Expand Down
9 changes: 7 additions & 2 deletions airbyte-ci/connectors/pipelines/pipelines/airbyte_ci/format/consts.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@

# TODO create .airbyte_ci_ignore files?
DEFAULT_FORMAT_IGNORE_LIST = [
AIRBYTE_SUBMODULE_DIR_NAME, # Required to format the airbyte-enterprise repo.
"**/__init__.py", # These files has never been formatted and we don't want to start now (for now) see https://github.com/airbytehq/airbyte/issues/33296
"**/__pycache__",
"**/.eggs",
Expand Down Expand Up @@ -44,6 +43,12 @@
"airbyte-ci/connectors/pipelines/tests/test_format/non_formatted_code", # This is a test directory with badly formatted code
]

# In the airbyte-enterprise repo, the airbyte repo is included as a submodule.
# Glob patterns which don't start with '**/' need to be repeated with a prefix.
DEFAULT_FORMAT_IGNORE_LIST = DEFAULT_FORMAT_IGNORE_LIST + [
f"{AIRBYTE_SUBMODULE_DIR_NAME}/{path}" for path in DEFAULT_FORMAT_IGNORE_LIST if not path.startswith("**/")
]


class Formatter(Enum):
"""An enum for the formatter values which can be ["java", "js", "python", "license"]."""
Expand All @@ -62,7 +67,7 @@ class Formatter(Enum):
WARM_UP_INCLUSIONS = {
Formatter.JAVA: [
"spotless-maven-pom.xml",
"tools/gradle/codestyle/java-google-style.xml",
"tools/", # Whole directory instead of just 'java-google-style.xml' to support airbyte-enterprise.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why this change is required. And I think it could impact performance as we mount more things during the "warm up" phase.

Copy link
Contributor Author

@postamar postamar Aug 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In airbyte-enterprise we have a symlink to tools and including a file inside of that only works if the symlink works.

I understand why you'd be worried but tools only contains 1.2 MB of files which basically never change. I think it's OK.

],
Formatter.PYTHON: ["pyproject.toml", "poetry.lock"],
Formatter.LICENSE: [LICENSE_FILE_NAME],
Expand Down
29 changes: 16 additions & 13 deletions airbyte-ci/connectors/pipelines/pipelines/airbyte_ci/format/containers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,11 @@
# Copyright (c) 2023 Airbyte, Inc., all rights reserved.
#

from typing import Any, Dict, List, Optional
from typing import Any, Dict, List, Optional, Union

import dagger
from pipelines.airbyte_ci.format.consts import CACHE_MOUNT_PATH, DEFAULT_FORMAT_IGNORE_LIST, REPO_MOUNT_PATH, WARM_UP_INCLUSIONS, Formatter
from pipelines.consts import GO_IMAGE, MAVEN_IMAGE, NODE_IMAGE, PYTHON_3_10_IMAGE
from pipelines.consts import AIRBYTE_SUBMODULE_DIR_NAME, GO_IMAGE, MAVEN_IMAGE, NODE_IMAGE, PYTHON_3_10_IMAGE
from pipelines.helpers import cache_keys
from pipelines.helpers.utils import sh_dash_c

Expand Down Expand Up @@ -69,26 +69,32 @@ def build_container(
return container


def warmup_directory(dagger_client: dagger.Client, formatter: Formatter) -> Union[dagger.Directory, None]:
"""Creates a Dagger directory which includes the warmup files for the given Formatter."""
include = WARM_UP_INCLUSIONS.get(formatter)
if not include:
return None
include = include + [f"{AIRBYTE_SUBMODULE_DIR_NAME}/{i}" for i in include]
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we check thaht AIRBYTE_SUBMODULE_DIR_NAME exists first? I'm not sure if dagger fails if you tell it to include not existing path (as the submodule does not exists in the main repo)

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, conveniently enough, it's fine if the path doesn't exist!

return dagger_client.host().directory(".", include=include, exclude=DEFAULT_FORMAT_IGNORE_LIST)


def format_java_container(dagger_client: dagger.Client, java_code: dagger.Directory) -> dagger.Container:
"""
Create a Maven container with spotless installed with mounted code to format and a cache volume.
We warm up the container cache with the spotless configuration and dependencies.
"""
warmup_dir = dagger_client.host().directory(
".",
include=WARM_UP_INCLUSIONS[Formatter.JAVA],
exclude=DEFAULT_FORMAT_IGNORE_LIST,
)
return build_container(
dagger_client,
base_image=MAVEN_IMAGE,
warmup_dir=warmup_dir,
warmup_dir=warmup_directory(dagger_client, Formatter.JAVA),
install_commands=[
# Run maven before mounting the sources to download all its dependencies.
# Dagger will cache the resulting layer to minimize the downloads.
# The go-offline goal purportedly downloads all dependencies.
# This isn't quite the case, we still need to add spotless goals.
"mvn -f spotless-maven-pom.xml"
" --errors"
" --batch-mode"
" org.apache.maven.plugins:maven-dependency-plugin:3.6.1:go-offline"
" spotless:apply"
" spotless:check"
Expand All @@ -111,13 +117,12 @@ def format_js_container(dagger_client: dagger.Client, js_code: dagger.Directory,

def format_license_container(dagger_client: dagger.Client, license_code: dagger.Directory) -> dagger.Container:
"""Create a Go container with addlicense installed with mounted code to format."""
warmup_dir = dagger_client.host().directory(".", include=WARM_UP_INCLUSIONS[Formatter.LICENSE], exclude=DEFAULT_FORMAT_IGNORE_LIST)
return build_container(
dagger_client,
base_image=GO_IMAGE,
dir_to_format=license_code,
install_commands=["go get -u github.com/google/addlicense"],
warmup_dir=warmup_dir,
warmup_dir=warmup_directory(dagger_client, Formatter.LICENSE),
)


Expand All @@ -127,8 +132,6 @@ def format_python_container(
"""Create a Python container with pipx and the global pyproject.toml installed with mounted code to format and a cache volume.
We warm up the container with the pyproject.toml and poetry.lock files to not repeat the pyproject.toml installation.
"""

warmup_dir = dagger_client.host().directory(".", include=WARM_UP_INCLUSIONS[Formatter.PYTHON], exclude=DEFAULT_FORMAT_IGNORE_LIST)
return build_container(
dagger_client,
base_image=PYTHON_3_10_IMAGE,
Expand All @@ -140,7 +143,7 @@ def format_python_container(
"poetry install --no-root",
],
dir_to_format=python_code,
warmup_dir=warmup_dir,
warmup_dir=warmup_directory(dagger_client, Formatter.PYTHON),
# Namespacing the cache volume by black version is likely overkill:
# Black already manages cache directories by version internally.
# https://github.com/psf/black/blob/e4ae213f06050e7f76ebcf01578c002e412dafdc/src/black/cache.py#L42
Expand Down
6 changes: 3 additions & 3 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 @@ -12,7 +12,8 @@
from pipelines import main_logger
from pipelines.airbyte_ci.format.actions import list_files_in_directory
from pipelines.airbyte_ci.format.configuration import Formatter
from pipelines.airbyte_ci.format.consts import DEFAULT_FORMAT_IGNORE_LIST, REPO_MOUNT_PATH, WARM_UP_INCLUSIONS
from pipelines.airbyte_ci.format.consts import DEFAULT_FORMAT_IGNORE_LIST, REPO_MOUNT_PATH
from pipelines.airbyte_ci.format.containers import warmup_directory
from pipelines.consts import GIT_IMAGE
from pipelines.helpers import sentry_utils
from pipelines.helpers.cli import LogOptions, log_command_results
Expand Down Expand Up @@ -170,8 +171,7 @@ async def run_format(
"""
format_container = container.with_exec(sh_dash_c(format_commands), skip_entrypoint=True)
formatted_directory = format_container.directory(REPO_MOUNT_PATH)
if warmup_inclusion := WARM_UP_INCLUSIONS.get(self.formatter):
warmup_dir = dagger_client.host().directory(".", include=warmup_inclusion, exclude=DEFAULT_FORMAT_IGNORE_LIST)
if warmup_dir := warmup_directory(dagger_client, self.formatter):
not_formatted_code = not_formatted_code.with_directory(".", warmup_dir)
formatted_directory = formatted_directory.with_directory(".", warmup_dir)
return (
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.30.0"
version = "4.30.1"
description = "Packaged maintained by the connector operations team to perform CI for connectors' pipelines"
authors = ["Airbyte <[email protected]>"]

Expand Down
Loading