feat(ingestion): strike the Pulumi steps, add ol-dbt inventory drift - #2608
Open
blarghmatey wants to merge 4 commits into
Open
feat(ingestion): strike the Pulumi steps, add ol-dbt inventory drift#2608blarghmatey wants to merge 4 commits into
blarghmatey wants to merge 4 commits into
Conversation
Three measurements taken while starting the Pulumi work moved the balance, and none of them favours building it: 1. Airbyte's API masks connector secrets server-side (confirmed against the live workspace). An imported source reads the mask into Pulumi state while the declared configuration holds the real value, so §6.4's acceptance test — an empty preview after import — is unreachable for 33 of 50 sources. 2. Database sources authenticate with Vault DYNAMIC roles, which mint a new credential per read. Declaring one rotates the database user on every apply and guarantees a dirty preview, so the static-role switch would have had to land first. 3. The workaround for (1) is `ignore_changes` on `configuration`, after which Pulumi can no longer push a rotated password into Airbyte either. The one benefit that would have justified the stack alone is the one the workaround removes. Against that, every connection here is scheduled for deletion: the stack was always designed to be destroyed unit by unit as sources move to dlt (§6.5), so it is an investment in a shrinking asset, and the migration is what actually retires the risk. §6 is kept rather than deleted — it is the record of why, and the starting point if this is revisited. None of it was wrong; it buys less than it costs. Step 8's drift check gets MORE important, not less: with the configuration hand-managed, a scheduled diff of live Airbyte against the inventory is the only thing that notices a UI edit, and `render airbyte` is already the right shape to compare against. §4 and §5 are re-pointed at that consumer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
With steps 5-6 struck, nothing applies the inventory to Airbyte, so the file can quietly stop describing reality and nothing would notice. This is the check that does: diff a workspace dump against `render airbyte`, which is already the inventory expressed in Airbyte's own shape. Takes a saved dump rather than reading the API, so the credentialed step stays separate and a report can be re-derived offline. Severity follows what a finding says about the inventory. A declaration the workspace does not honour means the inventory is WRONG — a model may be reading a table nothing loads — and is an ERROR: a missing connection, a dropped stream, a changed sync_mode or cursor, a live prefix outside the declared one, a status mismatch. Something live the inventory merely does not cover is a WARNING: usually config that should have been deleted, harmless until something depends on it. ITS FIRST RUN FOUND A REAL ERROR IN THE MERGED INVENTORY. The six edxorg units were hand-written because the generator could not express the deployment, and their sync_mode/cursor_field came from a default in that script rather than from Airbyte. Seven streams declared `full_refresh_overwrite` with no cursor that Airbyte actually runs `incremental_append` on `_ab_source_file_last_modified`, plus one primary_key. Corrected here by re-deriving the three fields from the snapshot instead of by hand a second time. Two things the first run got wrong, both fixed before the data: * `table_prefix` is compared with `startswith`, not equality. It is declared documentation covering a unit's tables (§1.1), not a copy of the literal string Airbyte prepends — the mongodb units legitimately declare `raw__<dep>__openedx__mongodb__` while Airbyte writes `…__mongodb__forum_`. Drift is a live prefix the declared one no longer covers. * An Airbyte-side schedule on a PAUSED connection is a warning, not an error. It cannot double-schedule anything while paused; resuming it would. That second one also corrects §8.1, which recorded "connections carrying their own Airbyte cron: 0". Three do — Mailgun legacy, GitHub, HubSpot Bootcamps — all paused, which is presumably why the original count missed them. Baseline is now 0 errors, 5 warnings, and all five are deliberate: the three connections pending deletion and the two paused ones carrying a schedule. What remains for step 8 is scheduling it and choosing how it reports. Dagster already holds the Airbyte basic-auth credential, which argues for a Dagster schedule over a Concourse pipeline. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🔎 ol-dbt impact — column-level blast radius✅ No column-level downstream impact detected for the changed models. Posted by |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
Adds Airbyte inventory drift detection while formally abandoning the Pulumi management steps.
Changes:
- Adds and tests
ol-dbt inventory drift. - Corrects edx.org stream metadata from the live snapshot.
- Documents the decision to keep Airbyte hand-managed.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/ol_dbt_cli/tests/test_inventory_drift.py |
Tests drift scenarios and severities. |
src/ol_dbt_cli/ol_dbt_cli/lib/inventory.py |
Implements drift comparison logic. |
src/ol_dbt_cli/ol_dbt_cli/commands/inventory.py |
Adds the drift CLI command. |
ingestion/inventory/units/edxorg__tracking_logs.yml |
Corrects incremental synchronization metadata. |
ingestion/inventory/units/edxorg__s3.yml |
Corrects S3 stream synchronization metadata. |
ingestion/inventory/units/edxorg__google_sheets.yml |
Corrects the composite primary key. |
docs/specs/INGESTION_INVENTORY_SPEC.md |
Records the Pulumi decision and drift-check status. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Five findings, each verified against the workspace before acting. * Source-level state was not compared at all. `render_airbyte` carries `source_kind` and `replication_method` per unit and every connection has a `sourceId`, but the check reduced everything to streams — so flipping a source from xmin to a cursor column produced zero drift while §3.4's whole purpose quietly went wrong. That field is what tk-determine-per-source-incremental-cursor-viabilit-51f299 reads. * Comparing `table_prefix` against Airbyte's `prefix` cannot work in either direction, and the falsey-prefix guard made it worse: clearing a prefix on a database connection moves every table it lands and was skipped silently. Replaced with `prefix + stream` against the declared `raw_table`, which reproduces all 949 declared raw tables exactly across the live workspace and handles the twelve prefixless S3 connections without a special case. * A missing `configurations.streams` was read as an empty stream list. The dumper re-fetches a connection whose list response omitted its streams but leaves the key absent when that GET also fails, so one transient API failure would have reported every declared stream in the workspace as dropped. Now an incomplete snapshot says so and stops; an explicitly empty list is still drift. * The summary counted units, not connections: `render airbyte` skips the nine dlt and Dagster units, so "43 live against 43 units" read as agreement while three live connections went undeclared. Now 43 against 40. * The snapshot was parsed twice, the second time outside the error handler. The per-connection raw-table map is keyed by connection rather than globally, because a stream name is only unique within its unit — `users` alone belongs to three Mongo forums and to Zendesk. Getting that wrong is what made the first version of this analysis report 346 false mismatches. Baseline is unchanged: 0 errors, 5 warnings, all five deliberate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Completes step 8. Daily at 03:00 UTC, which is the acceptance criterion stated rather than a number picked — "a connection edited in the UI is reported within a day" — and ahead of the ingestion schedules, so a report describes the workspace as it was configured for that day's syncs. Dagster rather than Concourse because it already holds the Airbyte basic-auth credential the check needs, and because a failing run already routes to Sentry through the existing run-failure sensor. An ERROR therefore needs no reporting path of its own. Reads the raw API responses rather than `fetch_airbyte_workspace_data`. dagster-airbyte's `AirbyteConnection` carries only id, name, stream_prefix and stream names — no status, no schedule, no `sourceId`, no per-stream sync mode or cursor — which is most of what drift means here, so the library's model cannot carry the check. Two refusals, both preferring no answer to a wrong one: * An empty read fails the run instead of being compared. Every declared connection would otherwise be reported as deleted, turning one bad fetch into a page-worthy alarm. * A connection whose stream config the list response omitted is re-fetched, the same fallback bin/airbyte-inventory.py has. If that also fails, `check_drift` reports an unusable snapshot rather than claiming every declared stream was dropped. Warnings do not fail the run. An undeclared connection is usually config nobody deleted, and paging on it would train people to ignore this. Gated on SKIP_AIRBYTE alongside the Airbyte assets: the asset requires the `airbyte` resource, which is not registered under that flag, and a definition asking for an absent resource fails the whole code location at load. lakehouse gains an ol-dbt-cli dependency for `ol_dbt_cli.lib.inventory`. That module imports neither dbt nor duckdb precisely so a Dagster code location can read the inventory (§5), and the image already installs it via the root project — the declaration makes an implicit dependency explicit rather than adding one. NOT verified end to end: the code location does not load locally without a starrocks dbt manifest, which needs warehouse credentials, so the fetch path is unexercised against a live workspace until this runs in Dagster. The adaptation, both refusals and the pass/fail outcomes are covered by seven tests against a fake client. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What are the relevant tickets?
Steps 5, 6 and 8 of
docs/specs/INGESTION_INVENTORY_SPEC.md, under RFC https://github.com/mitodl/hq/discussions/12319. No GitHub issue tracks these directly.Description (What does it do?)
Two related changes: a decision not to build something, and the check that decision makes load-bearing.
Steps 5 and 6 are struck — Airbyte's configuration stays hand-managed until Airbyte is retired. §6.0 records why. Three things measured while starting the work:
ignore_changesonconfiguration, after which Pulumi can no longer push a rotated password into Airbyte either — removing the one benefit that would have justified the stack on its own.Against that, every connection here is scheduled for deletion: the stack was always designed to be destroyed unit by unit as sources move to dlt (§6.5). §6 is kept rather than deleted — it is the record of why, and the starting point if this is ever revisited.
ol-dbt inventory drift(step 8) is what keeps the file honest instead. It diffs a workspace dump againstrender airbyte, which is already the inventory expressed in Airbyte's own shape. It takes a saved dump rather than reading the API, so the credentialed step stays separate and a report can be re-derived offline.Severity follows what a finding says about the inventory:
sync_modeor cursor, a status mismatch, a live prefix outside the declared one.Its first run found a real error in the merged inventory. The six edxorg units were hand-written because the generator could not express that deployment, and their
sync_mode/cursor_fieldcame from a default in that script rather than from Airbyte. Seven streams declaredfull_refresh_overwritewith no cursor that Airbyte actually runsincremental_appendon_ab_source_file_last_modified, plus one wrong primary key. Corrected here by re-deriving those fields from the snapshot rather than hand-writing them again.Two things the check itself got wrong on that first run, both fixed before the data was touched:
table_prefixcompares withstartswith, not equality. It is declared documentation covering a unit's tables (§1.1), not a copy of the literal string Airbyte prepends — the mongodb units legitimately declareraw__<dep>__openedx__mongodb__while Airbyte writes…__mongodb__forum_.That second fix also corrects §8.1, which recorded "connections carrying their own Airbyte cron: 0". Three do — Mailgun legacy, GitHub, HubSpot Bootcamps — all paused, which is presumably why the original count missed them.
How can this be tested?
All credential-free, all run on this branch:
The 5 warnings are the whole current baseline and every one is deliberate:
retired.yml, plus the pausededx.org Production Course Metadatasuperseded bydg_projects/edxorg. All three are being deleted in Airbyte.To reproduce against the live workspace, take a dump first — it is read-only, every call is a GET:
Not run: nothing was applied to Airbyte, and no Pulumi preview was taken — this PR removes the need for both.
Additional Context
driftis not wired into CI, deliberately: it needs a workspace dump CI does not have. What remains for step 8 is scheduling it and choosing how it reports, against the spec's "within a day". Dagster already holds the Airbyte basic-auth credential (secret-airbyte/dagster), which argues for a Dagster schedule over a Concourse pipeline — worth deciding in review.Work that was done and discarded rather than landed here: a generated
sdks/airbytePulumi SDK and a delete/replace preview gate in ol-infrastructure, plus a sources-artifact generator here. All unpushed; the ol-infrastructure branch is kept locally in case the decision is revisited. Also deliberately not landed: a narrowing ofSENSITIVE_KEY_MARKERSinbin/airbyte-inventory.pythat would have un-redactedauth_type,database_config.auth_sourceandentra_service_principal_auth. Those are ordinary configuration rather than credentials, but with no consumer reading them the conservative redaction is the better default. §6.0 notes it so the over-redaction is not rediscovered as a bug.