test(ingestion): e2e for Task Delivery tasks_completed (jira) + connector enrich hook - #1470
Conversation
…ctor enrich hook
Add a declarative YAML e2e test for the Task Delivery `tasks_completed` metric
(IC bullet …0011) driven entirely from bronze_jira, plus the generic connector
"enrich" step the rig needs to run it end to end.
- e2e_lib/enrich.py: discover connectors that declare `images.enrich` in their
descriptor.yaml, cargo-build the Rust enrich binary (--features io) and run it
between the staging and silver dbt builds — mirrors prod
(dbt(tag:<c>) -> <c>-enrich -> dbt(silver)). Data-driven; no per-connector code.
- specs/test_fixtures.py: run triggered enrich steps, then build the silver fed
by seeded bronze (derive_selectors) PLUS the silver produced from each enrich
step's ephemeral staging output (dbt_runner.ephemeral_silver_targets), by name
— avoids dragging unseeded streams / the identity chain into a minimal seed.
- create-bronze-placeholders.sh: extend bronze_jira.jira_issue (source_id,
jira_id, created, parent_id, project_key, reporter_id) and add jira_user,
jira_issue_history, jira_fields placeholders, mirroring what the jira staging
models and the enrich binary actually read.
- specs/: schemas + templates + task_delivery_tasks_completed_jira.test.yaml
seeding bronze_jira.{jira_issue,jira_issue_history,jira_fields,jira_user};
asserts tasks_completed = 2 for the filtered person.
- conftest.py: enrich_runner fixture; reset task-tracking silver/staging tables
at session start for warm-run determinism.
- jira__issue_field_snapshot.sql: move max_bytes_before_external_* from table
SETTINGS to query_settings — ClickHouse rejects them as storage settings
(code 115 UNKNOWN_SETTING on 24.8); they are query-execution settings.
Verified: ./e2e.sh test -k task_delivery_tasks_completed_jira -> 1 passed.
YouTrack tasks_completed follows in a separate PR.
Signed-off-by: Roman Mitasov <Roman.Mitasov@constructor.tech>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Jira bronze placeholder tables and schemas, wires an enrich runner into the e2e pipeline, updates the runner image build to use prebuilt binaries, and adds a JIRA task-completed metric spec. It also moves two ClickHouse external memory settings into ChangesJira enrich e2e pipeline
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ingestion/scripts/create-bronze-placeholders.sh`:
- Around line 704-720: The schema update for bronze_jira.jira_issue is only
applied when the table does not exist, so existing placeholders never get the
new contract. Update create-bronze-placeholders.sh around ch_table_exists
bronze_jira jira_issue to also handle pre-existing tables by ALTERing
bronze_jira.jira_issue to add any missing required columns before proceeding.
Keep the CREATE TABLE path for fresh installs, but ensure the jira_issue
placeholder is migrated to the current column set whenever the script runs.
In `@src/ingestion/tests/e2e/specs/test_fixtures.py`:
- Around line 64-80: Only expand the silver target set with ephemeral outputs
from enrich steps that actually executed. In the fixture logic around
enrich_steps, track which step names were run when discover_source_ids returns
results and use that filtered set when calling
dbt_runner.ephemeral_silver_targets(step.name), so skipped enrich steps do not
add unsupported silver models to silver_set.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7fb80824-75ba-4325-990e-3b8f6809684a
📒 Files selected for processing (12)
src/ingestion/connectors/task-tracking/jira/dbt/jira__issue_field_snapshot.sqlsrc/ingestion/scripts/create-bronze-placeholders.shsrc/ingestion/tests/e2e/conftest.pysrc/ingestion/tests/e2e/e2e_lib/dbt_runner.pysrc/ingestion/tests/e2e/e2e_lib/enrich.pysrc/ingestion/tests/e2e/specs/schemas/bronze_jira.jira_fields.yamlsrc/ingestion/tests/e2e/specs/schemas/bronze_jira.jira_issue.yamlsrc/ingestion/tests/e2e/specs/schemas/bronze_jira.jira_issue_history.yamlsrc/ingestion/tests/e2e/specs/schemas/bronze_jira.jira_user.yamlsrc/ingestion/tests/e2e/specs/task_delivery_tasks_completed_jira.test.yamlsrc/ingestion/tests/e2e/specs/templates/jira_task.yamlsrc/ingestion/tests/e2e/specs/test_fixtures.py
| found.update(str(r[0]) for r in rows) | ||
| return sorted(found) | ||
|
|
||
| def ensure_built(self, step: EnrichStep, *, timeout_s: float = 600.0) -> None: |
There was a problem hiding this comment.
Why tests build something? If tests require some binaries - use them from build step
There was a problem hiding this comment.
The test run compiles nothing. The enrich binary is built once at the image-build step (./e2e.sh build): the build-only jira-enrich compose service compiles it from the connector's own prod Dockerfile, and the runner bakes it in via additional_contexts + COPY --from. enrich.py only locates the on-PATH binary (shutil.which) and runs it — no toolchain in the runner, nothing compiled at test time. So the binary does come from the build step, as you suggested; this comment predates that refactor.
- test_fixtures: only add an enrich step's ephemeral silver targets when the step actually ran (had a source_id); otherwise we'd build silver depending on enrich output that was never produced. - create-bronze-placeholders.sh: reconcile a pre-existing bronze_jira.jira_issue placeholder via ALTER … ADD COLUMN IF NOT EXISTS (source_id, jira_id, created, parent_id, project_key, reporter_id) so a warm ClickHouse picks up the new column contract that a CREATE TABLE IF NOT EXISTS alone never adds. - enrich.py: add missing docstrings (EnrichError, binary_path, __init__, ensure_built) to clear the docstring-coverage warning. Re-verified: ./e2e.sh test -k task_delivery_tasks_completed_jira -> 1 passed. Signed-off-by: Roman Mitasov <Roman.Mitasov@constructor.tech>
…go-in-python The rig no longer compiles the enrich binary itself. Instead `e2e.sh build` builds each connector's enrich image FROM ITS OWN Dockerfile (the same one that ships the prod image — no duplicated build recipe) and extracts the compiled binary into tests/e2e/target/enrich/<bin>; the runner just executes the staged binary between the staging and silver dbt builds. Why: keeps build logic in one place (the connector Dockerfile), removes cargo orchestration from the Python harness, and avoids docker-in-docker — `e2e.sh` runs on the host (which has the Docker daemon), the runner container does not. - enrich.py: drop the cargo build (`ensure_built`, the `_resolve_cargo` import); EnrichStep now carries the connector's dockerfile/context; `run()` executes the pre-staged binary and fails clearly if it is missing. Add `python -m e2e_lib.enrich --plan` so the build side and run side share one descriptor-driven discovery. - e2e.sh: `stage_enrich_binaries()` in `build` — discovers enrich steps via the runner (python+pyyaml), then `docker build` + `docker cp` each binary out by its image ENTRYPOINT path. Data-driven; a new connector participates by declaring images.enrich, no rig change. - .gitignore: ignore tests/e2e/target/ (dbt + staged enrich binaries). Re-verified: ./e2e.sh build && ./e2e.sh test -k task_delivery_tasks_completed_jira -> 1 passed. Signed-off-by: Roman Mitasov <Roman.Mitasov@constructor.tech>
Replace the host-side `docker build` + `docker cp` staging with a compose-native bake, addressing review: docker-compose.runner.yml now carries the wiring and the binary lives IN the runner image (not mounted from a side dir). - docker-compose.runner.yml: add a build-only `jira-enrich` service that compiles the binary FROM THE CONNECTOR'S OWN Dockerfile (no build-recipe duplication), and wire it into the runner via `build.additional_contexts: jira-enrich: service:…`. - Dockerfile.runner: `COPY --from=jira-enrich /usr/local/bin/jira-enrich …` bakes it onto PATH. One service + one additional_context + one COPY per enrich connector. - enrich.py: drop all build/staging (cargo, docker, --plan). It now only discovers the steps from descriptors and RUNS the on-PATH binary for the connector whose bronze the test seeded (per-test gating stays — a jira test must not run youtrack-enrich), scoped to the seeded source_id. - e2e.sh: `build` is just `docker compose build runner`; its additional_contexts pull + bake the enrich binary. No docker-in-docker, no host staging step. Re-verified: ./e2e.sh build && ./e2e.sh test -k task_delivery_tasks_completed_jira -> 1 passed (enrich binary baked in, run from PATH). Signed-off-by: Roman Mitasov <Roman.Mitasov@constructor.tech>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/ingestion/tests/e2e/e2e_lib/enrich.py (1)
160-178: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTimeout escapes as
subprocess.TimeoutExpired, bypassingEnrichErrordiagnostics.When the enrich binary exceeds
timeout_s,subprocess.runraisessubprocess.TimeoutExpiredrather than theEnrichErrorpath below, so callers get an inconsistent exception type and lose the stdout/stderr tail captured on the timeout exception.♻️ Wrap timeout into EnrichError
- result = subprocess.run( - [ - step.binary, - "--insight-source-id", sid, - "--clickhouse-host", self.cfg.ch_host, - "--clickhouse-port", str(self.cfg.ch_http_port), - "--clickhouse-user", self.cfg.ch_user, - ], - env=env, - capture_output=True, - text=True, - check=False, - timeout=timeout_s, - ) + try: + result = subprocess.run( + [ + step.binary, + "--insight-source-id", sid, + "--clickhouse-host", self.cfg.ch_host, + "--clickhouse-port", str(self.cfg.ch_http_port), + "--clickhouse-user", self.cfg.ch_user, + ], + env=env, + capture_output=True, + text=True, + check=False, + timeout=timeout_s, + ) + except subprocess.TimeoutExpired as e: + stdout = (e.stdout or b"").decode() if isinstance(e.stdout, bytes) else (e.stdout or "") + stderr = (e.stderr or b"").decode() if isinstance(e.stderr, bytes) else (e.stderr or "") + raise EnrichError( + f"{step.name} enrich timed out after {timeout_s}s for source_id={sid}:\n" + f"stdout tail:\n{stdout[-1500:]}\nstderr tail:\n{stderr[-1500:]}" + ) from e🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ingestion/tests/e2e/e2e_lib/enrich.py` around lines 160 - 178, The timeout path in the enrich subprocess call bypasses the existing EnrichError handling because subprocess.run can raise subprocess.TimeoutExpired before the returncode check runs. Update the subprocess.run call in enrich.py’s enrichment flow to catch TimeoutExpired and rethrow EnrichError from the same enrich step context, preserving the timeout details plus any available stdout/stderr tail so callers always get consistent diagnostics from the step logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/ingestion/tests/e2e/e2e_lib/enrich.py`:
- Around line 160-178: The timeout path in the enrich subprocess call bypasses
the existing EnrichError handling because subprocess.run can raise
subprocess.TimeoutExpired before the returncode check runs. Update the
subprocess.run call in enrich.py’s enrichment flow to catch TimeoutExpired and
rethrow EnrichError from the same enrich step context, preserving the timeout
details plus any available stdout/stderr tail so callers always get consistent
diagnostics from the step logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 60aacaba-acc9-49aa-bb5f-6c47b073f32b
📒 Files selected for processing (5)
src/ingestion/tests/e2e/.gitignoresrc/ingestion/tests/e2e/compose/Dockerfile.runnersrc/ingestion/tests/e2e/compose/docker-compose.runner.ymlsrc/ingestion/tests/e2e/e2e.shsrc/ingestion/tests/e2e/e2e_lib/enrich.py
✅ Files skipped from review due to trivial changes (3)
- src/ingestion/tests/e2e/e2e.sh
- src/ingestion/tests/e2e/compose/Dockerfile.runner
- src/ingestion/tests/e2e/.gitignore
… enrich) analytics-api now follows the exact same pattern as the connector enrich binaries: compiled FROM ITS OWN Dockerfile and baked into the runner image, instead of being `cargo build`-ed inside the runner at session start. The rig no longer compiles anything, so the runner image carries no Rust toolchain. - docker-compose.runner.yml: add a build-only `analytics-api` service (src/backend + services/analytics-api/Dockerfile) and wire it into the runner via `additional_contexts: analytics-api: service:analytics-api`; drop the now unused cargo-cache / cargo-target volumes. - Dockerfile.runner: `COPY --from=analytics-api /app/analytics-api …` next to the enrich COPY; remove the rustup toolchain and the cargo-only apt deps (build-essential, pkg-config, protobuf-compiler, libssl-dev, curl). Image is now python + pre-built binaries only. - analytics_api.py: replace `build()` (cargo) with `locate_binary()` — finds the baked /usr/local/bin/analytics-api (PATH / host-mode cargo target fallback); drop the cargo resolver + version-check helpers. spawn/health-check unchanged. - conftest.py: use locate_binary(). - e2e-bronze-to-api.yml: drop the dead cargo actions/cache step (nothing compiles in the runner now). NOTE: component builds aren't wired to a cross-run BuildKit cache yet — full compile per CI run (follow-up: buildx type=gha cache). Re-verified: ./e2e.sh build && ./e2e.sh test -k task_delivery_tasks_completed_jira -> 1 passed (analytics-api + enrich baked, spawned/run from /usr/local/bin). Signed-off-by: Roman Mitasov <Roman.Mitasov@constructor.tech>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/e2e-bronze-to-api.yml:
- Line 45: The checkout step in the e2e-bronze-to-api workflow is leaving git
credentials persisted by default, which should be disabled for this job. Update
the actions/checkout@v4 step to explicitly turn off persisted credentials unless
a later step in the same job truly needs git authentication, and keep the change
scoped to that checkout usage in the workflow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bdf6534d-699e-48e8-8e6c-acbd5378c358
📒 Files selected for processing (5)
.github/workflows/e2e-bronze-to-api.ymlsrc/ingestion/tests/e2e/compose/Dockerfile.runnersrc/ingestion/tests/e2e/compose/docker-compose.runner.ymlsrc/ingestion/tests/e2e/conftest.pysrc/ingestion/tests/e2e/e2e_lib/analytics_api.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/ingestion/tests/e2e/compose/docker-compose.runner.yml
| @@ -44,18 +44,14 @@ jobs: | |||
| steps: | |||
| - uses: actions/checkout@v4 | |||
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Disable persisted checkout credentials.
actions/checkout leaves the token in the workspace by default. This job builds image contexts from the repo, so set persist-credentials: false unless a later step truly needs git auth.
🔧 Proposed fix
- - uses: actions/checkout@v4
+ - uses: actions/checkout@v4
+ with:
+ persist-credentials: false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - uses: actions/checkout@v4 | |
| - uses: actions/checkout@v4 | |
| with: | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 45-54: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 45-45: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/e2e-bronze-to-api.yml at line 45, The checkout step in the
e2e-bronze-to-api workflow is leaving git credentials persisted by default,
which should be disabled for this job. Update the actions/checkout@v4 step to
explicitly turn off persisted credentials unless a later step in the same job
truly needs git authentication, and keep the change scoped to that checkout
usage in the workflow.
Source: Linters/SAST tools
…ery-tasks-completed-jira Signed-off-by: Roman Mitasov <Roman.Mitasov@constructor.tech> # Conflicts: # src/ingestion/tests/e2e/specs/test_fixtures.py
| # prod image, build context src/backend). Consumed by the runner via | ||
| # additional_contexts (COPY --from) and baked in — the runner no longer compiles | ||
| # anything, so its image carries no Rust toolchain. | ||
| analytics-api: |
There was a problem hiding this comment.
Why analytics api overriden here?
There was a problem hiding this comment.
Same baking approach as the enrich binary: analytics-api is compiled from its own prod Dockerfile in a build-only service and baked into the runner via COPY --from. This keeps the runner image free of a Rust toolchain (nothing compiles at test time) and builds analytics-api identically to prod. It is a build-only service — never started, only consumed as a build context.
| # | ||
| # Adding another connector's enrich = one more service here + one | ||
| # `additional_contexts` entry + one `COPY --from` line in Dockerfile.runner. | ||
| jira-enrich: |
There was a problem hiding this comment.
Should it reside also in docker compose for local run?
There was a problem hiding this comment.
It is only needed to build the runner image, and e2e.sh always composes both files (-f docker-compose.yml -f docker-compose.runner.yml), so local runs already pick it up. Adding it to the base compose would expose a build-only service to a plain docker compose up with no benefit. Happy to consolidate if you'd prefer a single file.
jira__issue_field_snapshot emitted the due date as field_id='due_date' (underscore), but the changelog stream and jira_fields use Jira's native 'duedate', and task_issue_current_state filters field_id='duedate'. As a result a due date set only in the issue snapshot (at creation, never changed via the changelog) was silently dropped from due_date_compliance. Emit the native 'duedate' so snapshot-set due dates reach the metric. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Roman Mitasov <Roman.Mitasov@constructor.tech>
Two isolation defects surfaced by running two jira task-delivery tests back-to-back (previously the single tasks_completed test only ever ran in isolation, masking both): 1. truncate_touched ran TRUNCATE TABLE on view-materialized staging models (jira__task_users, jira__bronze_promoted are materialized='view'), which ClickHouse rejects with code 48 NOT_IMPLEMENTED. _truncate now looks up the engine and skips Views (stateless; the bronze underneath is truncated). 2. The enrich binary APPENDS into staging.jira__task_field_history, a dbt source (not a model) that dbt never rebuilds. Silver class_task_field_history is truncated via the ledger but rebuilt from the accumulating staging table, so rows piled up across tests and inflated absolute-count metrics (tasks_completed read 10 instead of 2). dbt_runner.enrich_output_tables resolves these tables from the ephemeral silver models' source() deps, and test_fixtures truncates them before each enrich run. Generic, no per-connector hardcoding. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Roman Mitasov <Roman.Mitasov@constructor.tech>
… (jira) Two bronze-only e2e tests for the Task Delivery IC bullet (...0011), both fed by the insight.jira_closed_tasks branch: - bugs_to_task_ratio: Alice closes 4 issues (1 Bug, 3 Task) -> 100*1/4 = 25.0. - due_date_compliance: 3 issues with a due date, 2 closed on time -> 66.7. The due date is set on the snapshot only (no duedate changelog item), so this test also guards against a regression of the snapshot field_id fix. All assertion-driving fields (issuetype via custom_fields_json, due_date, close event) are written inline in each test; the templates supply only invariant scaffolding. Asserts value only (cohort median/range need unseeded HR org_unit). Full e2e suite: 38 passed; all three jira task-delivery tests pass together. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Roman Mitasov <Roman.Mitasov@constructor.tech>
Resolve _SESSION_START_TRUNCATE conflict in e2e conftest.py by keeping both the zoom staging entries (main) and the task-tracking class_task_*/jira__* entries (this branch). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Roman Mitasov <Roman.Mitasov@constructor.tech>
What
Adds a declarative YAML e2e test for the Task Delivery
tasks_completedmetric (IC bullet…0011), driven entirely from bronze_jira, and the generic connector enrich step the rig needs to run it end to end.Verified locally:
./e2e.sh test -k task_delivery_tasks_completed_jira→ 1 passed.Why
silver.class_task_field_history(whichtasks_completedultimately reads) is produced by the Rustjira-enrichbinary, not dbt. The e2e rig previously ran onlydbt + migrations + analytics-api, so a bronze-only task-tracker test could not pass. This wires enrich into the rig the same way prod sequences it.Changes
e2e_lib/enrich.py(new): discover connectors that declareimages.enrichin theirdescriptor.yaml,cargo build --features iothe enrich binary, and run it between the staging and silver dbt builds — mirrors proddbt(tag:<c>) → <c>-enrich → dbt(silver). Fully data-driven; no per-connector code (YouTrack joins automatically once it ships animages.enrich).specs/test_fixtures.py+dbt_runner.ephemeral_silver_targets: after enrich, build the silver fed by seeded bronze (derive_selectors) plus the silver produced from each enrich step's ephemeral staging output, by name — so a minimal seed doesn't drag in unseeded streams (class_task_sprints, the identity chain) that would fail on absent bronze.create-bronze-placeholders.sh: extendbronze_jira.jira_issue(source_id,jira_id,created,parent_id,project_key,reporter_id) and addjira_user/jira_issue_history/jira_fieldsplaceholders — mirroring exactly what the jira staging models and the enrich binary read.specs/: schemas + templates +task_delivery_tasks_completed_jira.test.yamlseedingbronze_jira.{jira_issue,jira_issue_history,jira_fields,jira_user}; assertstasks_completed = 2for the filtered person.conftest.py:enrich_runnerfixture; reset task-tracking silver/staging tables at session start for warm-run determinism.jira__issue_field_snapshot.sql: movemax_bytes_before_external_*from tableSETTINGStoquery_settings. ClickHouse rejects them as storage settings (code 115UNKNOWN_SETTINGon 24.8); they are query-execution settings. Pre-existing bug surfaced by the rig actually building this model.Follow-up
YouTrack
tasks_completed(same metric, same generic hook) lands in a separate PR.🤖 Generated with Claude Code
Summary by CodeRabbit
source_idexecution.max_bytes_before_external_*as query settings.