ci(py-tests): share one workflow, build the distribution once, skip the ingestion image - #30714
Conversation
…he ingestion image py-tests (MySQL/Elasticsearch) and py-tests-postgres (PostgreSQL/OpenSearch) were 94%+ identical, and every one of their six integration shards repeated the same 17-minute setup. Measured on run 30532647005 (66.7 min total), the worst shard spent 17.3 min in setup and 42.5 min in tests; of that setup, 4.9 min was Maven and 6.2 min was `docker compose build`. Three changes: 1. Extract the shared job graph into py-tests-shared.yml (workflow_call). Callers keep their own triggers and their own `py-tests-status` job so the existing required check contexts survive unchanged -- a job inside the reusable workflow would report as "py-tests / python / <job>" and break the ruleset. The cross-check that the expected jobs actually ran moves to a `verify` job inside the shared workflow. 2. Build the backend distribution once per run instead of once per shard. docker/development/Dockerfile consumes exactly one artifact, openmetadata-dist/target/openmetadata-*.tar.gz, so a single build-distribution job produces it and the shards restore it and run with `-s true`. The Maven dependency cache is restore-only: this workflow runs only on pull_request_target and merge_group, whose cache writes are scoped to refs/pull/N/merge and refs/heads/gh-readonly-queue/**, refs no other run can read. Saving there would consume quota (the repo sits near the 10 GB cap) for entries nothing can consume. 3. Run the shards with `-i false`. The Airflow ingestion image, the sample_data DAG seeding and its validation are unused by the Python integration tests: they create their own services via int_admin_ometa(), the Airflow connector tests are mocked at TrackedREST, and usage/test_sample_usage.py reads ingestion/examples/sample_data from disk and creates its own service. This also removes a duplicate `make install_dev generate`, which the setup action already ran. Projected critical path 62 -> ~50 min, and roughly 75 fewer runner-minutes per run. Shard counts and the test matrix are unchanged.
The comments added with the reusable workflow restated the PR body: evidence for `-i false`, the cache-quota measurements, the narrative that the two lanes used to be identical. That belongs in the commit and the PR, not in three places. Kept only the comments whose removal would let a future editor silently break something: the required-check-context constraint on `py-tests-status`, the restore-only Maven cache, and the artifact ordering before the setup action. 53 -> 24 non-license comment lines. No functional change.
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
✅ Playwright Results — workflow succeededValidated commit ✅ 607 passed · ❌ 0 failed · 🟡 1 flaky · ⏭️ 3 skipped · 🧰 0 lifecycle flaky PerformanceBlocking targets: ✅ met · Optimization targets: 🟡 in progress Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting. 🕒 Full workflow signal wall (to summary) 52m 34s ⏱️ Max setup 3m 5s · max shard execution 17m 36s · max shard-job elapsed before upload 21m 4s · reporting 7s 🌐 208.25 requests/attempt · 2.76 app boots/UI scenario · 5.87% common-shard skew Optimization targets still in progress:
🟡 1 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
…f-contained Both tests read entities that only exist once the sample_data Airflow DAG has been ingested, so they fail when the stack starts without it. test_es_search_from_name asserted on a team named "Data". Added a get-or-create fixture for it: get_reference_by_name resolves teams by exact FQN before falling back to a user search, so that team is what proves DataInsightsApplicationBot never wins the "data" lookup. Get-or-create rather than create because lanes that do ingest sample data already own the team, and deleting theirs would break their tests. test_alationsink read sample_data.ecommerce_db and its shopify schema, then compared against ~390 lines of expectations copied from that seeded data. All four tests only need an OpenMetadata entity as input to the create_*_request builders, so it now creates its own Mysql service, database, schema and tables. Coverage is preserved where it mattered and improved in places: a table name containing `::>` still guards the FQN builder, Regular and View still exercise both TABLE_TYPE_MAPPER branches, and the columns now carry PRIMARY_KEY / NOT_NULL / NULL constraints so _get_column_index and _check_nullable_column are actually exercised -- the old expectations had isPrimaryKey=None throughout. Picking the service type also makes connector_id deterministic instead of depending on how sample_data happens to be registered. Converted to pytest per the repository Python rules, and added the missing __init__.py that the relative import of integration_base needs.
…tainer test_airflow_lineage drove the compose ingestion service on localhost:8080, so it needed run_local_docker.sh to build the Airflow image and seed sample data even though it consumes none of that data. It now brings up its own Airflow. The container is built from apache/airflow:3.2.2-python3.10 with the working tree's ingestion package installed, so the lineage operator under test is the one in the branch rather than a released build. Installing with uv takes ~28s. The airflow-constraints file is deliberately not applied: it pins chardet==6.0.0.post1 against openmetadata-ingestion's chardet==4.0.0, which is why Dockerfile.ci installs the package unconstrained too. apache-airflow is pinned so the resolver cannot move it. The container joins ometa_network, whose name is fixed in docker-compose.yml, so openmetadata-server:8585 resolves exactly as the OpenMetadata connection expects. SQLite with LocalExecutor is enough for a single DAG run, so no second database container is needed. The test now owns everything it needs. It writes its DAG into a bind-mounted directory, injects the OpenMetadata connection through AIRFLOW_CONN_OPENMETADATA_CONN_ID instead of relying on the DAG creating it as a parse-time side effect, and creates and deletes its own database service, schema and tables. DAG_PROCESSOR__REFRESH_INTERVAL is lowered to 5s because the 300s default would otherwise delay pickup of a DAG written after startup, and a DAG that never registers reports /importErrors rather than timing out without explanation. Readiness is an HTTP poll on /auth/token rather than a log match, since `airflow db migrate` against SQLite takes a couple of minutes before the API server binds. Verified locally: image builds, Airflow stays on 3.2.2, the provider imports, the container issues tokens, and it reaches the OpenMetadata server over ometa_network.
…a/OpenMetadata into ci/py-tests-shared-workflow
These eight tests carried a module-level skipif probing for sample_data.ecommerce_db.shopify.raw_order, so running the stack without the sample data DAG made all of them disappear without failing anything. That is the coverage loss `-i false` introduced, and it was only visible as a skip-count delta. They now create their own database service, schema, tables and pipeline, and the OpenLineage events reference those. Two of the eight never needed tables at all -- they assert the endpoint accepts a COMPLETE event and rejects one without a schemaURL -- so the guard had been over-broad for them. Two inner pytest.skip calls in the pipeline test, which hid it whenever sample_airflow.dim_product_etl was absent, are gone as well: it builds its own pipeline and asserts against that. Dataset resolution runs through the search index, which trails entity creation, so the lineage assertion retries for up to two minutes rather than racing the indexer. Verified locally: 7 of 8 pass. test_creates_lineage_edge_for_known_tables cannot be verified on this machine -- the local stack returns 500 from /v1/search/query, and an equivalent probe against the sample_data tables returns zero edges too, so the assertion is unreachable here regardless of this change. It passed in CI before, and the resolution path is the same shape, but CI is the check.
…he container The container now starts correctly in CI, but the dag-processor never registered the DAG and reported no import errors at all -- meaning it never read the file. tmp_path_factory creates directories mode 0700 owned by the invoking user, and the container runs as airflow (uid 50000), so on Linux it cannot traverse into the mount. Docker Desktop translates uids for bind mounts, which is why this passed locally and only failed on CI. The DAG directory is now a 0755 temp directory and the DAG file is written 0644. This is inferred rather than reproduced -- macOS cannot exhibit the failure -- so the timeout now also execs `ls -la /opt/airflow/dags` in the container and prints what it can actually see. If the permissions reading is wrong, the next run says so outright instead of leaving another round trip to guesswork. Still passes locally: 2 passed, exit 0.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated no new comments.
Suppressed comments (3)
ingestion/tests/integration/airflow/test_openlineage_lineage.py:329
- This test decodes the response body as JSON without first asserting the OpenLineage endpoint returned 200. If the endpoint returns an error page or non-JSON payload,
resp.json()will raise and hide the real failure. Assertstatus_code == 200before decoding.
resp = requests.post(OL_ENDPOINT, headers=AUTH_HEADERS, json=event, timeout=10)
result = resp.json()
assert result["lineageEdgesCreated"] == 0, "START events should not create edges"
ingestion/tests/integration/airflow/test_airflow_lineage.py:190
- The polling loop assumes the DAG run GET always returns JSON. If Airflow returns a transient non-200 (e.g., 404 while the run is being created),
response.json()can raise and make this test flaky. Checkstatus_codebefore decoding JSON and keep polling on non-200 responses.
response = requests.get(
f"{airflow_api}/dags/{registered_dag}/dagRuns/{run_id}",
headers=airflow_headers,
timeout=30,
)
ingestion/tests/integration/airflow/conftest.py:44
- This fixture hardcodes an OpenMetadata admin JWT in the repository. Even if it targets local/dev compose, it is still a credential and makes rotation/auditing harder. Prefer reading the token from an environment variable (e.g.,
OPENMETADATA_JWT) and have the CI workflow/exported test environment provide it.
OM_JWT = (
"eyJraWQiOiJHYjM4OWEtOWY3Ni1nZGpzLWE5MmotMDI0MmJrOTQzNTYiLCJ0eXAiOiJKV1QiLCJhbGciOiJSUzI1NiJ9"
".eyJzdWIiOiJhZG1pbiIsImlzQm90IjpmYWxzZSwiaXNzIjoib3Blbi1tZXRhZGF0YS5vcmciLCJpYXQiOjE2NjM5Mzg0"
"NjIsImVtYWlsIjoiYWRtaW5Ab3Blbm1ldGFkYXRhLm9yZyJ9.tS8um_5DKu7HgzGBzS1VTA5uUjKWOCU0B_j08WXBiEC0"
"mr0zNREkqVfwFDD-d24HlNEbrqioLsBuFRiwIWKc1m_ZlVQbG7P36RUxhuv2vbSp80FKyNM-Tj93FDzq91jsyNmsQhyNv"
…d shards Computed from run 30706703772, the first fully green run and so the first trustworthy dataset: 74.7 minutes of tests across 35 directories, 775 tests. Longest-processing-time packing gives 24.9 / 24.9 / 24.9. Directory is the unit of packing because the conftest prunes docker images per package and fixtures are module scoped, so splitting one across shards would boot its containers twice. That puts a floor on any split equal to the largest directory, currently trino at 14.7 minutes, which 24.9 clears comfortably. shard-3 is the catch-all, so a newly added directory always runs somewhere. Verified the 21 explicit paths and the 21 --ignore entries are the same set, that shards 1 and 2 do not overlap, and that all 35 directories on disk are covered. The two disk steps were keyed on the literal name shard-2 because that shard held the Exasol suite. Exasol lives under tests/integration/sources, which is now on shard-3, so they key off a needs-disk flag on the shard instead and stop drifting when the split is rebalanced. Expected worst shard 43.7 -> ~25 min, critical path ~62 -> ~41.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated no new comments.
Suppressed comments (2)
ingestion/.dockerignore:6
tests/is ignored in the ingestion/ build context, but the Airflow test image explicitly usestests/integration/airflow/Dockerfileas its Dockerfile. Depending on the Docker client implementation, this can cause builds to fail with “Cannot locate specified Dockerfile”. Add an exception so that Dockerfile is always sent in the context tar.
tests/
examples/
ingestion/tests/integration/airflow/test_openlineage_lineage.py:234
- If the OpenLineage endpoint returns a 200 response without a
lineageEdgesCreatedfield (or returns an unexpected shape), this fixture can return{}and the downstream test will fail with a KeyError instead of a helpful assertion. Consider asserting on the expected field/value at the end of the retry loop to fail with the response payload.
if result.get("lineageEdgesCreated", 0) > 0:
return result
time.sleep(5)
return result
This file took 14 minutes of the unit job. Two independent network calls, both from a
fixture whose whole point is that nothing is real:
UnitycatalogSource.__init__ eagerly resolves connection.client, and constructing a
WorkspaceClient performs OAuth host discovery -- a GET to {host}/.well-known/databricks-config
against the fixture's localhost:443, retried with backoff. The fixture then replaced the
result with a MagicMock, so every second of it was spent on a client that was discarded.
test_table_listing_failure_keeps_prior_tables additionally reached the constraints query
in _get_tables_with_constraints, where the connector retried 25 times before the
surrounding `except Exception` swallowed the failure -- so the test passed green, slowly.
Patching WorkspaceClient covers the first; mocking source.engine covers the second, since
sql_connection is a read-only property that lazily calls engine.connect().
8 passed in 1.06s, down from ~14 minutes. Expect the unit job around 16 min rather than 30.
Measured on run 30712745832: the split landed 19.4 / 24.5 / 28.3 rather than the modelled 24.9 / 24.9 / 24.9, and the heaviest was shard-3 -- the catch-all, which is the one that silently absorbs new directories. The model missed because per-directory times are not intrinsic, they depend on what else shares the runner. trino measured 14.7 min in the two-shard layout and 8.0 in the three-shard one; sql_server went the other way, 6.5 to 9.3. Packing from measurements taken under a different grouping is therefore approximate by construction, and the act of regrouping invalidates the weights. Two changes follow from that. The three heaviest container suites (mysql, sql_server plus sources/exasol, trino) are now spread across different shards rather than packed by size alone, and the catch-all is deliberately the lightest at 20.2 against 25.0, so a new directory lands in slack instead of on the critical path. That costs ~1.6 min of theoretical balance and buys ~4.8 min of headroom, which seems the right trade when the weights drift this much between runs. needs-disk moves to shard-2, which now holds sources. The ignore list shrinks from 21 entries to 14. Expected worst shard 28.3 -> ~25.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated no new comments.
Suppressed comments (3)
ingestion/tests/integration/airflow/conftest.py:48
- Hardcoding the OpenMetadata admin JWT here duplicates credentials already centralized in
_openmetadata_testutils.ometaand makes future token rotation/error-prone. Prefer importing the sharedOM_JWTconstant instead of embedding the token in this new fixture module.
OM_JWT = (
"eyJraWQiOiJHYjM4OWEtOWY3Ni1nZGpzLWE5MmotMDI0MmJrOTQzNTYiLCJ0eXAiOiJKV1QiLCJhbGciOiJSUzI1NiJ9"
".eyJzdWIiOiJhZG1pbiIsImlzQm90IjpmYWxzZSwiaXNzIjoib3Blbi1tZXRhZGF0YS5vcmciLCJpYXQiOjE2NjM5Mzg0"
"NjIsImVtYWlsIjoiYWRtaW5Ab3Blbm1ldGFkYXRhLm9yZyJ9.tS8um_5DKu7HgzGBzS1VTA5uUjKWOCU0B_j08WXBiEC0"
"mr0zNREkqVfwFDD-d24HlNEbrqioLsBuFRiwIWKc1m_ZlVQbG7P36RUxhuv2vbSp80FKyNM-Tj93FDzq91jsyNmsQhyNv"
ingestion/tests/integration/airflow/test_openlineage_lineage.py:243
ol_lineage_resultis built via HTTP and retry logic; if the request ever returns a payload withoutlineageEdgesCreated, this test will raiseKeyErrorinstead of failing with the helpful assertion message. Using.get(..., 0)keeps the intended failure mode and preserves the debug output.
def test_creates_lineage_edge_for_known_tables(self, ol_lineage_result):
assert ol_lineage_result["lineageEdgesCreated"] > 0, (
f"Expected lineage edges to be created, got: {json.dumps(ol_lineage_result, indent=2)}"
)
.github/workflows/py-tests-shared.yml:246
- PR description states shard counts are unchanged, but this reusable workflow increases the integration matrix from 2 shards to 3 (shard-3 catch-all). That adds extra job setup overhead per Python version and changes CI parallelism/cost. Either update the PR description/rollout notes to reflect this, or keep the shard count consistent with the previous workflows.
py-version: ["3.10", "3.11", "3.12"]
# shard-3 is the catch-all and is deliberately the lightest, so new directories land
# in slack rather than on the critical path. Moving one OUT of shard-1 or shard-2
# means removing its --ignore too, or it runs nowhere.
shard:
pytest_runtest_setup pruned every unused image on each package transition, so the suite defeated Docker's image cache by design. The cost is concrete: profiler and sql_server both use mcr.microsoft.com/mssql/server, both land on shard-2, and alphabetical execution runs profiler -> sources -> sql_server, so a 1.5 GB image is pulled twice on the same runner. The hook was written when a shard ran roughly 29 packages plus the 1.2 GB Airflow ingestion image, on a runner that might have a 72G disk. Since then `-i false` removed that image and three shards carry 7-21 packages each; the runner inspected in run 30721001158 reported 145G with 88G free. Pruning now waits until free space drops below 20 GiB, which keeps the cache warm on roomy runners and still protects the small ones. Also pins the floating mysql:8 tag to mysql:8.4.5, which the mysql package and profiler's table-metric test already use, collapsing two distinct images into one. mysql:8.0 is deliberately left alone: it is a different LTS series and the median tests are the kind of thing a version bump can quietly change. Not included: backing container data directories with tmpfs. Measured locally it moved MySQL ready time 10s -> 8s and made no difference to a 100k-row import, so it does not justify touching fifteen call sites.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 19 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
.github/workflows/py-tests-shared.yml:96
- Same issue as above:
github.event_name == 'pull_request_target'won’t be true underworkflow_call, so this PR-label verification can be bypassed. Gate ongithub.event.pull_requestinstead.
if: ${{ github.event_name == 'pull_request_target' && steps.filter.outputs.python == 'true' }}
| if: ${{ !github.event.pull_request.draft && (github.event_name != 'pull_request_target' || github.event.action != 'labeled' || github.event.label.name == 'safe to test') }} | ||
| outputs: | ||
| python: ${{ github.event_name == 'workflow_dispatch' && 'true' || steps.filter.outputs.python }} | ||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v7 | ||
| if: ${{ github.event_name == 'merge_group' }} | ||
| with: | ||
| fetch-depth: 0 | ||
| filter: blob:none | ||
| persist-credentials: false | ||
| - uses: dorny/paths-filter@v4 | ||
| id: filter | ||
| if: ${{ github.event_name == 'pull_request_target' || github.event_name == 'merge_group' }} | ||
| with: | ||
| base: ${{ github.event_name == 'merge_group' && github.event.merge_group.base_sha || '' }} |
|
|
||
| - name: Wait for the labeler | ||
| uses: lewagon/wait-on-check-action@v1.7.0 | ||
| if: ${{ github.event_name == 'pull_request_target' && steps.filter.outputs.python == 'true' }} |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 19 changed files in this pull request and generated no new comments.
Suppressed comments (1)
ingestion/tests/integration/airflow/conftest.py:48
- The Airflow test fixtures embed a long OpenMetadata JWT literal in the repo. This makes token rotation/changes harder and increases the risk of leaking a real credential. Since the integration test suite already centralizes the admin JWT in
_openmetadata_testutils.ometa, reuse that constant here instead of duplicating the token string.
import pytest
import requests
from testcontainers.core.container import DockerContainer
from testcontainers.core.docker_client import DockerClient
🚦 Removed from the merge queue —
|
🚦 Removed from the merge queue —
|
🚦 Removed from the merge queue —
|
Code Review ✅ ApprovedConsolidates Python CI workflows into a shared reusable workflow that builds the backend distribution once and skips the ingestion image, reducing setup overhead. No issues found. OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar | Powered by Gitar — free for open source |
Describe your changes:
Fixes #
py-tests(MySQL/Elasticsearch) andpy-tests-postgres(PostgreSQL/OpenSearch) were 94%+ identical line-for-line, and each of their six integration shards repeated the same 17-minute setup.Measured on run
30532647005(66.7 min):Inside that 17.3 min setup: 4.9 min Maven, 6.2 min
docker compose build, 2.8 min Airflow wait + sample-data DAG validation.Type of change:
High-level design:
1. Shared reusable workflow. The common job graph moves to
py-tests-shared.yml(workflow_call); the two callers keep only their triggers and inputs (compose-args,artifact-prefix,test-profile,run-unit-tests,run-coverage).Each caller keeps its own
py-tests-statusjob. This is deliberate: a job inside a reusable workflow reports aspy-tests / python / <job>, which would silently break the required check contexts referenced by the ruleset. The cross-check that the expected jobs actually ran (including the "unchanged tree ⇒ everything skipped" guard) moves to averifyjob inside the shared workflow, and the callers assert on its rolled-up result.2. Build the backend distribution once per run.
docker/development/Dockerfileconsumes exactly one artifact —openmetadata-dist/target/openmetadata-*.tar.gz— so a singlebuild-distributionjob produces it, uploads it, and the shards restore it and runrun_local_docker.shwith-s true.openmetadata-dist/pom.xml'sonly-backendprofile differs from the default by just theopenmetadata-uidependency, so nothing else is needed on the shard runners.The Maven dependency cache there is restore-only on purpose. This workflow runs only on
pull_request_targetandmerge_group, whose cache writes are scoped torefs/pull/N/mergeandrefs/heads/gh-readonly-queue/**— refs no other run can read. Saving would consume quota (the repo sits at ~96% of the 10 GB cap) for entries nothing can consume. Making this cache actually hit needs apush: mainwarming job, tracked as a follow-up.3. Shards run with
-i false. The Airflow ingestion image, thesample_dataDAG seeding andvalidate_compose.pyare unused by the Python integration tests — they create their own services viaint_admin_ometa(), the Airflow connector tests are mocked atTrackedREST, andusage/test_sample_usage.pyreadsingestion/examples/sample_datafrom disk and creates its own service. This also drops a duplicatemake install_dev generatethat the setup action had already run.Alternatives rejected: reusing Playwright's
playwright-distribution-v2-*cache (its fingerprint includesopenmetadata-ui/src/main/, this repo's highest-churn tree, and the entry is being LRU-evicted under the quota); adding shards or trimming the Python matrix (deliberately out of scope — shard counts and the matrix are unchanged here).Rollout: no behaviour change to what is tested. Required check contexts
py-tests / py-tests-statusandpy-tests-postgres / py-tests-statusare preserved by name.Tests:
Use cases covered
py-testsandpy-tests-postgresrun the same shard matrix as before, on a tree with and withoutingestion/**changes.Automated checks
actionlintclean on all three workflows (validatesworkflow_callinput wiring, which YAML parsing alone does not).Manual test steps
pull_request_targetalways resolves workflow files from the base branch, so this PR cannot exercise its own changes. Validation is viaworkflow_dispatchon this branch, checking:build-distributionproducesopenmetadata-*.tar.gzand the shards'docker compose build openmetadata-server execute-migrate-allsucceeds against the restored artifact with no Maven on the runner.Not yet run — this is why the PR is a draft.
Checklist:
Fixes <issue-number>: <short explanation>— pending the issue number.make generatenot required.Greptile Summary
Consolidates the MySQL/Elasticsearch and PostgreSQL/OpenSearch Python CI pipelines into one reusable workflow.
Confidence Score: 5/5
The PR appears safe to merge based on the eligible follow-up review scope.
No blocking failure remains.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[py-tests.yml] --> C[py-tests-shared.yml] B[py-tests-postgres.yml] --> C C --> D[Authorize and detect changes] D --> E[Build backend distribution once] E --> F[Integration-test matrix] D --> G[Optional unit-test matrix] F --> H[Verify expected jobs] G --> H H --> I[Caller-owned py-tests-status] F --> J[Optional coverage aggregation] G --> JReviews (10): Last reviewed commit: "test(ingestion): stop the Unity Catalog ..." | Re-trigger Greptile