Moving away from dbt core operation blocks - #1435
Conversation
|
Warning Review limit reached
Next review available in: 54 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughChangesThe PR centralizes dbt profile generation and runner Secret propagation, replaces multiple Elementary endpoints with consolidated check/install flows, expands EDR orchestration and report cleanup, adds deployment migration commands, and standardizes local dbt setup on dbt-core 1.10.19. DBT profile runtime
Elementary and EDR workflow
Operational migration and cleanup
DBT version standardization
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ddpui/services/org_cleanup_service.py (1)
260-290: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMove dbt-profile secret block cleanup to the transformation-layer teardown
delete_org()clearsself.org.dbtbeforedelete_warehouse()runs, so this lookup isNonein the normal full-org delete path. Ifdelete_warehouse()runs on its own, the same Prefect block can be deleted once perOrgWarehouse. Move this cleanup todelete_transformation_layer()whileorgdbtis still available, and run it once.🤖 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 `@ddpui/services/org_cleanup_service.py` around lines 260 - 290, The dbt-profile secret block cleanup currently occurs in the per-warehouse delete flow, after delete_org() may clear self.org.dbt and potentially repeats for each OrgWarehouse. Remove that cleanup from delete_warehouse() and move it into delete_transformation_layer(), using the available orgdbt record and executing it once while preserving the Prefect deletion, error logging, and OrgPrefectBlockv1 deletion behavior.
🧹 Nitpick comments (4)
ddpui/management/commands/sync_edr_secret_block.py (1)
112-117: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBroad except conflates "not found" with real failures.
Catching bare
Exceptionhere treats any failure fromget_secret_block_by_name(auth errors, network issues, Prefect outage) the same as "block doesn't exist," printing a benign[skip] ... not presentinstead of surfacing the real error. Elsewhere in the codebase (e.g.org_cleanup_service.py) not-found lookups catch the specificHttpErrorrather than a blanketException.♻️ Suggested narrower catch
try: block = prefect_service.get_secret_block_by_name(legacy_name) - except Exception: # pylint: disable=broad-exception-caught + except HttpError: # Block doesn't exist — nothing to delete. self.stdout.write(f" [skip] {legacy_name}: not present") continuePlease confirm
get_secret_block_by_name/prefect_getraisesHttpError(vs. some other exception type) on a 404 so the narrower catch doesn't miss the legitimate not-found case.🤖 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 `@ddpui/management/commands/sync_edr_secret_block.py` around lines 112 - 117, Replace the broad exception handler around get_secret_block_by_name with the specific HttpError type used by prefect_get for 404 responses, confirming that exception type from the client implementation or established org_cleanup_service.py pattern. Preserve the [skip] behavior only for not-found errors, while allowing authentication, network, and other failures to propagate.README.md (1)
146-152: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSibling-directory assumption is implicit.
The
cd ../prefect-proxy/docker/dbt-X.Y.Z/paths assumeprefect-proxyis cloned as a sibling directory to this repo, but Step 5 (which links to the prefect-proxy repo) doesn't say where to clone it relative to this repo. Worth a one-line clarification to avoid confusion for new setups.🤖 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 `@README.md` around lines 146 - 152, Clarify in Step 5 that the prefect-proxy repository must be cloned as a sibling directory of the current repository, matching the relative ../prefect-proxy/docker/dbt-X.Y.Z paths used in the dbt setup instructions.ddpui/tests/services/test_elementary_service.py (1)
881-1003: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTests correctly validate
install_elementary's step-wise progress emission and failure short-circuiting logic, but sincerun_dbt_commands.applyis mocked outright, they don't exercise the actual swallow-and-continue defect inrun_dbt_commandsflagged inddpui/celeryworkers/tasks.py(lines 313-376). Consider adding one integration-style test that calls the realrun_dbt_commands(with dbt subprocess calls mocked) to confirminstall_elementarystep 1 actually fails when dbt run fails.🤖 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 `@ddpui/tests/services/test_elementary_service.py` around lines 881 - 1003, The current tests mock run_dbt_commands.apply entirely and do not verify its real failure propagation. Add an integration-style test for install_elementary that invokes the actual run_dbt_commands implementation while mocking only its dbt subprocess calls to fail, then assert step 1 emits failed, step 2 is not called, and the exception propagates.ddpui/core/pipelinefunctions.py (1)
108-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
dbt_profile_secret_blockresolution logic across two functions.
setup_dbt_core_task_configandsetup_edr_send_report_task_configboth introduce the identical pattern of resolvingorg_task.org.dbt.dbt_profile_secret_blockand warning if absent. Consider extracting a small helper, e.g.resolve_dbt_profile_secret_block(org_task) -> str | None, to avoid future drift between the two warning messages/behaviors.♻️ Suggested helper
def _resolve_dbt_profile_secret_block(org_task: OrgTask, fail_hint: str) -> str | None: orgdbt = org_task.org.dbt if orgdbt and orgdbt.dbt_profile_secret_block: return orgdbt.dbt_profile_secret_block.block_name logger.warning( "OrgDbt for org=%s has no dbt_profile_secret_block — %s", org_task.org.slug, fail_hint, ) return NoneAlso applies to: 220-253
🤖 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 `@ddpui/core/pipelinefunctions.py` around lines 108 - 141, Extract the shared dbt profile secret block lookup from setup_dbt_core_task_config and setup_edr_send_report_task_config into a helper such as _resolve_dbt_profile_secret_block. Have it return the block name when available, otherwise log the warning with the supplied failure hint and return None; update both task configuration functions to use this helper while preserving their existing env and warning behavior.
🤖 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 `@ddpui/celeryworkers/tasks.py`:
- Around line 313-376: Update the outer exception handler in run_dbt_commands so
failures from profile writing or dbt clean/deps/run operations are re-raised
after recording the failure and updating progress. Ensure the task result is
marked failed, allowing install_elementary’s apply(...).maybe_throw() to stop
step 1 instead of emitting completed and scheduling reports.
In `@ddpui/management/commands/patch_edr_deployment_env.py`:
- Around line 58-88: Before constructing new_params in the deployment loop,
inspect each deployment’s existing config.tasks and detect pipelines containing
tasks beyond the regenerated EDR task. For mixed pipelines, skip the deployment
without calling update_dataflow_v1 (and report it consistently); only replace
config.tasks for deployments that contain exclusively the EDR task, preserving
dry-run and patched/skipped accounting.
---
Outside diff comments:
In `@ddpui/services/org_cleanup_service.py`:
- Around line 260-290: The dbt-profile secret block cleanup currently occurs in
the per-warehouse delete flow, after delete_org() may clear self.org.dbt and
potentially repeats for each OrgWarehouse. Remove that cleanup from
delete_warehouse() and move it into delete_transformation_layer(), using the
available orgdbt record and executing it once while preserving the Prefect
deletion, error logging, and OrgPrefectBlockv1 deletion behavior.
---
Nitpick comments:
In `@ddpui/core/pipelinefunctions.py`:
- Around line 108-141: Extract the shared dbt profile secret block lookup from
setup_dbt_core_task_config and setup_edr_send_report_task_config into a helper
such as _resolve_dbt_profile_secret_block. Have it return the block name when
available, otherwise log the warning with the supplied failure hint and return
None; update both task configuration functions to use this helper while
preserving their existing env and warning behavior.
In `@ddpui/management/commands/sync_edr_secret_block.py`:
- Around line 112-117: Replace the broad exception handler around
get_secret_block_by_name with the specific HttpError type used by prefect_get
for 404 responses, confirming that exception type from the client implementation
or established org_cleanup_service.py pattern. Preserve the [skip] behavior only
for not-found errors, while allowing authentication, network, and other failures
to propagate.
In `@ddpui/tests/services/test_elementary_service.py`:
- Around line 881-1003: The current tests mock run_dbt_commands.apply entirely
and do not verify its real failure propagation. Add an integration-style test
for install_elementary that invokes the actual run_dbt_commands implementation
while mocking only its dbt subprocess calls to fail, then assert step 1 emits
failed, step 2 is not called, and the exception propagates.
In `@README.md`:
- Around line 146-152: Clarify in Step 5 that the prefect-proxy repository must
be cloned as a sibling directory of the current repository, matching the
relative ../prefect-proxy/docker/dbt-X.Y.Z paths used in the dbt setup
instructions.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ddad13ab-628c-4814-8d9b-c58283f320ac
⛔ Files ignored due to path filters (3)
dbt_deps/dbt-1.10.19/uv.lockis excluded by!**/*.lockdbt_deps/dbt-1.8.7/uv.lockis excluded by!**/*.lockdbt_deps/dbt-1.9.8/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (31)
README.mddbt_deps/Dockerfile.prefect-job-runnerdbt_deps/README.mddbt_deps/dbt-1.10.19/pyproject.tomldbt_deps/dbt-1.8.7/pyproject.tomldbt_deps/dbt-1.9.8/pyproject.tomlddpui/api/dbt_api.pyddpui/api/orgtask_api.pyddpui/celeryworkers/tasks.pyddpui/core/dbtfunctions.pyddpui/core/orchestrate/pipeline_service.pyddpui/core/pipelinefunctions.pyddpui/ddpairbyte/airbytehelpers.pyddpui/ddpdbt/dbt_service.pyddpui/ddpdbt/dbthelpers.pyddpui/ddpdbt/elementary_service.pyddpui/management/commands/backfill_warehouse_secret_blocks.pyddpui/management/commands/ensureedrsendreportdataflow.pyddpui/management/commands/patch_edr_deployment_env.pyddpui/management/commands/sync_edr_secret_block.pyddpui/management/commands/updateedrsendreportdataflowtarget.pyddpui/migrations/0169_orgdbt_dbt_profile_secret_block_and_more.pyddpui/models/org.pyddpui/models/tasks.pyddpui/services/org_cleanup_service.pyddpui/tests/api_tests/test_dbt_api.pyddpui/tests/core/test_dbtfunctions.pyddpui/tests/core/test_pipelinefunctions.pyddpui/tests/services/test_elementary_service.pyddpui/utils/constants.pyddpui/utils/s3_utils.py
💤 Files with no reviewable changes (5)
- dbt_deps/dbt-1.9.8/pyproject.toml
- dbt_deps/dbt-1.8.7/pyproject.toml
- dbt_deps/README.md
- dbt_deps/dbt-1.10.19/pyproject.toml
- dbt_deps/Dockerfile.prefect-job-runner
| try: | ||
| dbt_project_params = DbtProjectManager.gather_dbt_project_params(org, orgdbt) | ||
| except Exception as err: # pylint: disable=broad-exception-caught | ||
| self.stdout.write(f" [skip] {org.slug}: gather_dbt_project_params failed ({err})") | ||
| skipped += 1 | ||
| continue | ||
|
|
||
| task_config = setup_edr_send_report_task_config(orgtask, dbt_project_params.project_dir) | ||
| new_params = { | ||
| "config": { | ||
| "tasks": [task_config.to_json()], | ||
| "org_slug": org.slug, | ||
| } | ||
| } | ||
|
|
||
| for dot in DataflowOrgTask.objects.filter(orgtask=orgtask).select_related("dataflow"): | ||
| dataflow = dot.dataflow | ||
| if options["dry_run"]: | ||
| self.stdout.write( | ||
| f" [dry-run] {dataflow.deployment_name}: would rebuild deployment_params" | ||
| ) | ||
| patched += 1 | ||
| continue | ||
|
|
||
| try: | ||
| update_dataflow_v1( | ||
| dataflow.deployment_id, | ||
| PrefectDataFlowUpdateSchema3( | ||
| deployment_params=new_params, cron=dataflow.cron | ||
| ), | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
# Map relevant files first
ast-grep outline ddpui/management/commands/patch_edr_deployment_env.py --view expanded || true
echo '---'
fd -a 'pipeline_service.py' .
echo '---'
fd -a 'dataflow*' ddpui | head -n 50
echo '---'
rg -n "send report|send-report|deployment_params|DataflowOrgTask|tasks" ddpui -g'*.py' | head -n 250Repository: DalgoT4D/DDP_backend
Length of output: 25476
🏁 Script executed:
set -euo pipefail
sed -n '70,210p' ddpui/core/orchestrate/pipeline_service.py
echo '---'
sed -n '430,520p' ddpui/ddpdbt/elementary_service.py
echo '---'
sed -n '150,330p' ddpui/core/orgtaskfunctions.pyRepository: DalgoT4D/DDP_backend
Length of output: 16034
🏁 Script executed:
set -euo pipefail
sed -n '1,220p' ddpui/management/commands/patch_edr_deployment_env.pyRepository: DalgoT4D/DDP_backend
Length of output: 4093
Guard against mixed EDR pipelines before rewriting deployment_params. ddpui/management/commands/patch_edr_deployment_env.py:58-88 replaces config.tasks with only the regenerated EDR task for every deployment attached to the orgtask. Orchestrated pipelines can already include EDR alongside other tasks, so this will drop the rest of the pipeline config. Skip those deployments or merge the existing tasks instead.
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 60-60: Do not catch blind exception: Exception
(BLE001)
🤖 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 `@ddpui/management/commands/patch_edr_deployment_env.py` around lines 58 - 88,
Before constructing new_params in the deployment loop, inspect each deployment’s
existing config.tasks and detect pipelines containing tasks beyond the
regenerated EDR task. For mixed pipelines, skip the deployment without calling
update_dataflow_v1 (and report it consistently); only replace config.tasks for
deployments that contain exclusively the EDR task, preserving dry-run and
patched/skipped accounting.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1435 +/- ##
==========================================
- Coverage 61.89% 61.62% -0.27%
==========================================
Files 152 152
Lines 17816 17976 +160
==========================================
+ Hits 11027 11078 +51
- Misses 6789 6898 +109 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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 `@ddpui/management/commands/backfill_dbt_profile_secret_blocks.py`:
- Around line 41-53: The warehouse-processing flow in the backfill command must
handle organizations with multiple OrgWarehouse rows before upserting the shared
dbt profile block. Group warehouses by organization, then either select the
established designated warehouse or skip/fail ambiguous organizations
explicitly; ensure each org’s dbt-profile-<org.slug> is written at most once.
- Around line 99-103: Update the migration flow around
_patch_deployments_for_org so deployment patch failures are returned or
aggregated instead of only logged. Increment ok and report the organization as
migrated only after patching succeeds; propagate any failure through the
command’s final status so the command fails when any DBTCORE deployment cannot
be updated.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 472c5828-a2e2-4c46-8052-0a6ac43b927f
📒 Files selected for processing (4)
ddpui/api/orgtask_api.pyddpui/management/commands/backfill_dbt_profile_secret_blocks.pyddpui/tests/helper/test_airbytehelpers.pyddpui/tests/services/dbt_service/test_generate_manifest_json_for_dbt_project.py
| warehouses = OrgWarehouse.objects.select_related("org").all() | ||
| if options["org"]: | ||
| warehouses = warehouses.filter(org__slug=options["org"]) | ||
|
|
||
| total = warehouses.count() | ||
| if total == 0: | ||
| print("No warehouses found") | ||
| return | ||
|
|
||
| print(f"Processing {total} warehouse(s)") | ||
| ok, skipped, failed = 0, 0, 0 | ||
|
|
||
| for warehouse in warehouses: |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Handle organizations with multiple warehouses explicitly.
At Line 41, each warehouse is processed independently, but every iteration writes the same dbt-profile-<org.slug> block. Since OrgWarehouse does not enforce one row per org, a multi-warehouse org ends with whichever credential payload is processed last. Select a designated warehouse or fail/skip ambiguous orgs before upserting.
🤖 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 `@ddpui/management/commands/backfill_dbt_profile_secret_blocks.py` around lines
41 - 53, The warehouse-processing flow in the backfill command must handle
organizations with multiple OrgWarehouse rows before upserting the shared dbt
profile block. Group warehouses by organization, then either select the
established designated warehouse or skip/fail ambiguous organizations
explicitly; ensure each org’s dbt-profile-<org.slug> is written at most once.
| print(f" [ok] {org.slug}: {block.block_name}") | ||
| ok += 1 | ||
|
|
||
| # Patch deployments so DBTCORE task_configs carry the runner env key. | ||
| _patch_deployments_for_org(org) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not report an org as migrated when deployment patching fails.
ok is incremented before _patch_deployments_for_org, while Lines 157-158 only log update failures. The command can exit successfully even though DBTCORE tasks still lack dbt-profile-secret-block and will not use the new runner secret. Return/aggregate patch failures and mark the org/command failed when any deployment cannot be updated.
Also applies to: 148-158
🤖 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 `@ddpui/management/commands/backfill_dbt_profile_secret_blocks.py` around lines
99 - 103, Update the migration flow around _patch_deployments_for_org so
deployment patch failures are returned or aggregated instead of only logged.
Increment ok and report the organization as migrated only after patching
succeeds; propagate any failure through the command’s final status so the
command fails when any DBTCORE deployment cannot be updated.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@ddpui/management/commands/migrate_deployments_to_runner_flow.py`:
- Around line 60-62: Update the dry-run branch in the deployment migration
command so it does not increment patched; instead increment a separate planned
counter, initialize and report that counter in the summary, while keeping
patched reserved for deployments actually modified.
- Around line 29-31: Update the queryset in the migration command before
iteration to exclude OrgDataFlowv1 records with a null deployment_id, while
preserving the existing optional org slug filter and select_related("org")
behavior.
In `@README.md`:
- Line 154: Update the fenced environment-variable code block in README.md to
declare the dotenv language by changing its opening fence to ```dotenv, while
preserving the block’s existing contents.
- Around line 144-159: Update the setup instructions around the uv sync command
to define and export DBT_VENV before invoking it, and retain the later .env
configuration with consistent path semantics. Ensure a fresh shell does not
expand an unset DBT_VENV into an unintended /venv destination.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 88b7a375-097d-4a71-8b78-15e6eb398826
📒 Files selected for processing (3)
README.mdddpui/ddpprefect/prefect_service.pyddpui/management/commands/migrate_deployments_to_runner_flow.py
| qs = OrgDataFlowv1.objects.select_related("org") | ||
| if options["org"]: | ||
| qs = qs.filter(org__slug=options["org"]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Skip dataflows without a deployment ID.
OrgDataFlowv1.deployment_id is nullable, so these rows call get_deployment(None) and generate avoidable proxy failures. Filter them before iterating.
Proposed fix
- qs = OrgDataFlowv1.objects.select_related("org")
+ qs = (
+ OrgDataFlowv1.objects.select_related("org")
+ .exclude(deployment_id__isnull=True)
+ .exclude(deployment_id="")
+ )📝 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.
| qs = OrgDataFlowv1.objects.select_related("org") | |
| if options["org"]: | |
| qs = qs.filter(org__slug=options["org"]) | |
| qs = ( | |
| OrgDataFlowv1.objects.select_related("org") | |
| .exclude(deployment_id__isnull=True) | |
| .exclude(deployment_id="") | |
| ) | |
| if options["org"]: | |
| qs = qs.filter(org__slug=options["org"]) |
🤖 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 `@ddpui/management/commands/migrate_deployments_to_runner_flow.py` around lines
29 - 31, Update the queryset in the migration command before iteration to
exclude OrgDataFlowv1 records with a null deployment_id, while preserving the
existing optional org slug filter and select_related("org") behavior.
| if options["dry_run"]: | ||
| self.stdout.write(f" [dry-run] {label}: {OLD_ENTRYPOINT} → {NEW_ENTRYPOINT}") | ||
| patched += 1 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not report dry-run operations as patches.
Incrementing patched here makes the final summary claim deployments were patched despite --dry-run performing no writes. Track a separate planned count.
🤖 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 `@ddpui/management/commands/migrate_deployments_to_runner_flow.py` around lines
60 - 62, Update the dry-run branch in the deployment migration command so it
does not increment patched; instead increment a separate planned counter,
initialize and report that counter in the summary, while keeping patched
reserved for deployments actually modified.
| All orgs run on dbt-core 1.10.19. Set up a local venv from the pinned pyproject.toml: | ||
|
|
||
| #### For DBT 1.8.7 (current production) | ||
| ```bash | ||
| cd dbt_deps/dbt-1.8.7/ | ||
| cd ../prefect-proxy/docker/dbt-1.10.19/ | ||
| UV_PROJECT_ENVIRONMENT=$DBT_VENV/venv uv sync | ||
| ``` | ||
|
|
||
| #### For DBT 1.9.8 (migration target) | ||
| ```bash | ||
| cd dbt_deps/dbt-1.9.8/ | ||
| UV_PROJECT_ENVIRONMENT=$DBT_VENV/venv-1.9.8 uv sync | ||
| ``` | ||
| The `uv.lock` file is committed for reproducible versions. | ||
|
|
||
| #### For DBT 1.10.19 | ||
| ```bash | ||
| cd dbt_deps/dbt-1.10.19/ | ||
| UV_PROJECT_ENVIRONMENT=$DBT_VENV/venv-1.10.19 uv sync | ||
| ``` | ||
|
|
||
| **Note**: The `uv.lock` files are committed to ensure reproducible dependency versions across all environments. | ||
|
|
||
| Set the DBT environment path in `.env`: | ||
| Set the DBT paths in `.env`: | ||
| ``` | ||
| CLIENTDBT_ROOT=/path/to/client/dbt/projects | ||
| DBT_VENV=/path/to/dbt/environments | ||
| ``` | ||
|
|
||
| This creates the structure: | ||
| ``` | ||
| $DBT_VENV/ | ||
| ├── venv/ # DBT 1.8.7 | ||
| └── venv-1.9.8/ # DBT 1.9.8 | ||
| ``` | ||
|
|
||
| Organizations use either `venv` or `venv-1.9.8` in their `dbt_venv` database field. | ||
| Django resolves the dbt binary as `$DBT_VENV/<orgdbt.dbt_venv>/bin/dbt` (default `dbt_venv` value is `venv`). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Set DBT_VENV before running uv sync.
The command uses $DBT_VENV before the document defines it, and .env values are not automatically available in the shell. A fresh setup can target /venv instead. Define/export it before the command.
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 154-154: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@README.md` around lines 144 - 159, Update the setup instructions around the
uv sync command to define and export DBT_VENV before invoking it, and retain the
later .env configuration with consistent path semantics. Ensure a fresh shell
does not expand an unset DBT_VENV into an unintended /venv destination.
|
|
||
| Set the DBT environment path in `.env`: | ||
| Set the DBT paths in `.env`: | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Declare the .env fence language.
Use ```dotenv to satisfy the configured Markdown lint rule.
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 154-154: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@README.md` at line 154, Update the fenced environment-variable code block in
README.md to declare the dotenv language by changing its opening fence to
```dotenv, while preserving the block’s existing contents.
Source: Linters/SAST tools
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
ddpui/management/commands/update_dbt_binary_path.py (4)
177-184: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPreserve the existing
PATHentries.Replacing
PATHwith only the new virtualenv directory can remove system paths and other required tooling from the EDR task. Prependedr_bin_dirto the existing path usingos.pathsep, rather than discardingold_path.🤖 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 `@ddpui/management/commands/update_dbt_binary_path.py` around lines 177 - 184, Update the EDR task PATH handling in the TASK_GENERATE_EDR branch to preserve existing entries: construct the new PATH by prepending edr_bin_dir to old_path with os.pathsep, while handling an empty old_path without an unnecessary separator. Compare and assign this combined value, and keep the existing modified flag and output behavior.
186-214: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not report successful migration after deployment failures.
The broad handler logs an error and continues, while
OrgDbt.dbt_venvhas already been saved. The command can therefore leave the database and Prefect deployments inconsistent, increment the success counters, and exit successfully. Catch only expected exceptions, record failures, and return a non-zeroCommandErrorafter processing.🤖 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 `@ddpui/management/commands/update_dbt_binary_path.py` around lines 186 - 214, The deployment loop’s broad exception handling allows failed updates to increment success counters and the command to exit successfully after OrgDbt.dbt_venv is saved. In the deployment-processing flow, catch only expected exceptions, record whether any deployment failed, prevent failed organizations from contributing to successful migration counters, and return a non-zero CommandError after processing all entries when failures occurred.Source: Linters/SAST tools
73-84: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftEnumerate organizations rather than
OrgDbtrecords.The all-org path currently skips every configured organization whose
dbt_venvisNULL, despite claiming to process all dbt-configured orgs. It also includes orphanedOrgDbtrows, andOrg.objects.get(dbt=org_dbt)can raiseMultipleObjectsReturnedbecauseOrg.dbtis a non-unique foreign key. QueryOrg.objects.filter(dbt__isnull=False)and iterate each organization directly.Also applies to: 104-111
🤖 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 `@ddpui/management/commands/update_dbt_binary_path.py` around lines 73 - 84, Update the organization resolution and iteration in the command to enumerate Org records directly: use Org.objects.filter(dbt__isnull=False) for the all-org path and retain the single-org validation for org_slug. Iterate organizations and use each organization’s dbt configuration, removing the OrgDbt-based filtering and reverse lookup so NULL dbt_venv and duplicate references are handled correctly.
64-71: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winValidate that the new path is an executable file.
Path.exists()also accepts directories and non-executable files, allowing the command to persist an unusable dbt path and fail later in Prefect.Proposed fix
- if not Path(new_binary_path).exists(): + if not Path(new_binary_path).is_file() or not os.access(new_binary_path, os.X_OK):🤖 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 `@ddpui/management/commands/update_dbt_binary_path.py` around lines 64 - 71, Update the new_dbt_venv validation in the command’s dry-run guard to require new_binary_path to be a regular executable file, not merely an existing path. Use the appropriate Path file and executable checks before allowing the command to proceed, while preserving the existing CommandError and message for invalid paths.
🤖 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.
Outside diff comments:
In `@ddpui/management/commands/update_dbt_binary_path.py`:
- Around line 177-184: Update the EDR task PATH handling in the
TASK_GENERATE_EDR branch to preserve existing entries: construct the new PATH by
prepending edr_bin_dir to old_path with os.pathsep, while handling an empty
old_path without an unnecessary separator. Compare and assign this combined
value, and keep the existing modified flag and output behavior.
- Around line 186-214: The deployment loop’s broad exception handling allows
failed updates to increment success counters and the command to exit
successfully after OrgDbt.dbt_venv is saved. In the deployment-processing flow,
catch only expected exceptions, record whether any deployment failed, prevent
failed organizations from contributing to successful migration counters, and
return a non-zero CommandError after processing all entries when failures
occurred.
- Around line 73-84: Update the organization resolution and iteration in the
command to enumerate Org records directly: use
Org.objects.filter(dbt__isnull=False) for the all-org path and retain the
single-org validation for org_slug. Iterate organizations and use each
organization’s dbt configuration, removing the OrgDbt-based filtering and
reverse lookup so NULL dbt_venv and duplicate references are handled correctly.
- Around line 64-71: Update the new_dbt_venv validation in the command’s dry-run
guard to require new_binary_path to be a regular executable file, not merely an
existing path. Use the appropriate Path file and executable checks before
allowing the command to proceed, while preserving the existing CommandError and
message for invalid paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3566d24f-08da-419f-890b-b52e1109eb21
📒 Files selected for processing (4)
ddpui/celeryworkers/tasks.pyddpui/management/commands/update_dbt_binary_path.pyddpui/migrations/0170_orgdbt_dbt_profile_secret_block.pyddpui/tests/services/test_elementary_service.py
🚧 Files skipped from review as they are similar to previous changes (1)
- ddpui/celeryworkers/tasks.py
2 participants
Commands/scripts to be run in this order
elementary_profiles/profiles.ymland run the report flow.Summary by CodeRabbit
New Features
Bug Fixes