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

🐛 source-file: fix integration tests #31152

Closed
wants to merge 5 commits into from
Closed

🐛 source-file: fix integration tests #31152

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8496a7d
try no timeout
pedroslopez Oct 3, 2023
4b67980
rm docker test reqs
pedroslopez Oct 3, 2023
108a094
bump dep, add docker to test container
pedroslopez Oct 6, 2023
ea8ea31
re-add timeout
pedroslopez Oct 6, 2023
63b8aaf
bump airbyte-ci version
pedroslopez Oct 6, 2023
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
11 changes: 6 additions & 5 deletions airbyte-ci/connectors/pipelines/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,17 +396,18 @@ This command runs the Python tests for a airbyte-ci poetry package.
`airbyte-ci tests airbyte-integrations/bases/connector-acceptance-test --test-directory=unit_tests`

## Changelog
| Version | PR | Description |
|---------| --------------------------------------------------------- |-----------------------------------------------------------------------------------------------------------|
| Version | PR | Description |
|---------| -------------------------------------------------------- |-----------------------------------------------------------------------------------------------------------|
| 1.5.0 |[#30984](https://github.com/airbytehq/airbyte/pull/30984) | Adds docker to python test containers |
| 1.4.6 |[ #31087](https://github.com/airbytehq/airbyte/pull/31087) | Throw error if airbyte-ci tools is out of date |
| 1.4.5 | [#31133](https://github.com/airbytehq/airbyte/pull/31133) | Fix bug when building containers using `with_integration_base_java_and_normalization`. |
| 1.4.4 | [#30743](https://github.com/airbytehq/airbyte/pull/30743) | Add `--disable-report-auto-open` and `--use-host-gradle-dist-tar` to allow gradle integration. |
| 1.4.3 | [#30595](https://github.com/airbytehq/airbyte/pull/30595) | Add --version and version check |
| 1.4.2 | [#30595](https://github.com/airbytehq/airbyte/pull/30595) | Remove directory name requirement |
| 1.4.1 | [#30595](https://github.com/airbytehq/airbyte/pull/30595) | Load base migration guide into QA Test container for strict encrypt variants |
| 1.4.0 | [#30330](https://github.com/airbytehq/airbyte/pull/30330) | Add support for pyproject.toml as the prefered entry point for a connector package |
| 1.3.0 | [#30461](https://github.com/airbytehq/airbyte/pull/30461) | Add `--use-local-cdk` flag to all connectors commands |
| 1.2.3 | [#30477](https://github.com/airbytehq/airbyte/pull/30477) | Fix a test regression introduced the previous version. |
| 1.3.0 | [#30461](https://github.com/airbytehq/airbyte/pull/30461) | Add `--use-local-cdk` flag to all connectors commands |
| 1.2.3 | [#30477](https://github.com/airbytehq/airbyte/pull/30477) | Fix a test regression introduced the previous version. |
| 1.2.2 | [#30438](https://github.com/airbytehq/airbyte/pull/30438) | Add workaround to always stream logs properly with --is-local. |
| 1.2.1 | [#30384](https://github.com/airbytehq/airbyte/pull/30384) | Java connector test performance fixes. |
| 1.2.0 | [#30330](https://github.com/airbytehq/airbyte/pull/30330) | Add `--metadata-query` option to connectors command |
Expand All @@ -432,7 +433,7 @@ This command runs the Python tests for a airbyte-ci poetry package.
| 0.2.1 | [#28767](https://github.com/airbytehq/airbyte/pull/28767) | Improve pytest step result evaluation to prevent false negative/positive. |
| 0.2.0 | [#28857](https://github.com/airbytehq/airbyte/pull/28857) | Add the `airbyte-ci tests` command to run the test suite on any `airbyte-ci` poetry package. |
| 0.1.1 | [#28858](https://github.com/airbytehq/airbyte/pull/28858) | Increase the max duration of Connector Package install to 20mn. |
| 0.1.0 | | Alpha version not in production yet. All the commands described in this doc are available. |
| 0.1.0 | | Alpha version not in production yet. All the commands described in this doc are available. |

## More info
This project is owned by the Connectors Operations team.
Expand Down
3 changes: 2 additions & 1 deletion airbyte-ci/connectors/pipelines/pipelines/actions/environments.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ def with_python_base(context: PipelineContext, python_version: str = "3.10") ->
sh_dash_c(
[
"apt-get update",
"apt-get install -y build-essential cmake g++ libffi-dev libstdc++6 git",
"apt-get install -y build-essential cmake g++ libffi-dev libstdc++6 git curl",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

required to be able to install docker

"pip install pip==23.1.2",
]
)
Expand All @@ -82,6 +82,7 @@ def with_testing_dependencies(context: PipelineContext) -> Container:

return (
python_environment.with_exec(["pip", "install"] + CONNECTOR_TESTING_REQUIREMENTS)
.with_exec(sh_dash_c(["curl -fsSL https://get.docker.com | sh"]))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

python test containers now include docker

Copy link
Contributor

Choose a reason for hiding this comment

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

So the use case in here is we want to be able to use docker in tests to spin up containers nessessary for certain test environments. e.g. source-file needs docker compose for its test "remote" file server

(@pedroslopez do I have that right?)

In any sense looking at the code we already have I see with_global_dockerd_service and with_docker_cli inside the environments.py mega file.

But I dont think those are useful here for installing docker-compose.

(@alafanechere can you fact check me here?)

So if thats all true then I think were on the right track but should have a modification

  1. Lets not use curl
  2. Lets install docker / docker-compose via apt-get

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI: In #30474 this with_testing_dependencies function is removed. Connectors test will run inside the connector container on top of which we'd install its testing dependencies according to its setup.py.
I believe docker as a system dependency should not be added to this function, nor to a future "pre/post" hook for source-file.

If we need docker-compose for testing I suggest to install the pip distributed docker-compose by declaring pytest-docker[docker-compose-v1] as a test requirement. see https://github.com/avast/pytest-docker .

If we really want to use the latest docker-compose version I suggest to only install the docker-compose plugin. This can be added to with_testing_dependencies in this PR but eventually added to a pre/post build hook following #30474 .

I'd prefer that we stick to the pip distributed docker compose as it will not require a pre/post build hook implementation for this connector. And I believe it's fine to use an old docker compose version in this context.

.with_file(f"/{PYPROJECT_TOML_FILE_PATH}", pyproject_toml_file)
.with_file(f"/{LICENSE_SHORT_FILE_PATH}", license_short_file)
)
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 = "1.4.6"
version = "1.5.0"
description = "Packaged maintained by the connector operations team to perform CI for connectors' pipelines"
authors = ["Airbyte <[email protected]>"]

Expand Down
2 changes: 1 addition & 1 deletion airbyte-integrations/connectors/source-file/setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
"pyxlsb==1.0.9",
]

TEST_REQUIREMENTS = ["requests-mock~=1.9.3", "pytest~=6.2", "pytest-docker~=1.0.0", "pytest-mock~=3.6.1", "docker-compose"]
TEST_REQUIREMENTS = ["requests-mock~=1.9.3", "pytest~=6.2", "pytest-docker~=2.0", "pytest-mock~=3.6.1"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

docker-compose is deprecated and was causing infinite loop when installing test dependencies. We now bump pytest-docker to v2, which removes usages of docker-compose in favor of the new docker compose.


setup(
name="source_file",
Expand Down
Loading