Conversation
…ers from all extra - Remove deprecated 'name' fields from package-lock.yml that dbt 1.8+ rejects - Remove dbt-clickhouse, dbt-sqlserver, dbt-vertica from 'all' extra as they pin dbt-core to 1.8.x (still available as individual extras) Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
…' fields Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
👋 @devin-ai-integration[bot] |
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThe PR updates package configurations by bumping the version to 0.23.1, removing two dbt packages from the aggregated Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
The 'name' field in package-lock.yml is not deprecated - it is the current format generated by modern dbt versions. The issue was specific to dbt 1.8.x which has a bug rejecting this field. The real fix is removing restrictive adapters from the 'all' extra so dbt resolves to 1.11+ where this bug doesn't exist. Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pyproject.toml (1)
79-79: Document the breaking change in release notes.Removing
dbt-clickhouse,dbt-fabricspark,dbt-sqlserver, anddbt-verticafrom theallextra is a scope change for users who installelementary-data[all]. Users relying on these adapters via theallextra will need to explicitly install them (e.g.,pip install elementary-data[clickhouse]).As noted in the PR checklist, consider adding a migration note to the v0.23.1 changelog to inform users of this change.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` at line 79, Update the release notes/changelog (v0.23.1) to document the breaking change where the pyproject.toml extras group all (the all = [...] entry) no longer includes dbt-clickhouse, dbt-fabricspark, dbt-sqlserver, and dbt-vertica; explicitly state that users who previously relied on elementarty-data[all] must now install those adapters separately (e.g., pip install elementary-data[clickhouse]) and add a short migration note and example command in the changelog entry to guide users.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pyproject.toml`:
- Line 79: Update the release notes/changelog (v0.23.1) to document the breaking
change where the pyproject.toml extras group all (the all = [...] entry) no
longer includes dbt-clickhouse, dbt-fabricspark, dbt-sqlserver, and dbt-vertica;
explicitly state that users who previously relied on elementarty-data[all] must
now install those adapters separately (e.g., pip install
elementary-data[clickhouse]) and add a short migration note and example command
in the changelog entry to guide users.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 971c4332-0c54-4f59-810c-d87c7f733ea4
📒 Files selected for processing (3)
.github/workflows/release.ymlelementary/monitor/dbt_project/package-lock.ymlpyproject.toml
- Remove 'name' fields from package-lock.yml (unsupported in dbt 1.8) - Re-add release validation with corrected wording: 'unsupported in dbt 1.8' instead of incorrectly saying 'deprecated' Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
Only dbt-vertica (dbt-core==1.8.5 exact pin) and dbt-fabricspark (broken upstream) need to stay out. dbt-clickhouse 1.10.0 requires dbt-core>=1.9 and dbt-sqlserver 1.9.0 requires dbt-core>=1.9.0,<2.0, so both are compatible with newer dbt-core versions. Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
… all extra Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
.github/workflows/release.yml
Outdated
| print("packages.yml validation passed - using proper package version reference.") | ||
| EOF | ||
|
|
||
| - name: Validate package-lock.yml has no 'name' fields unsupported in dbt 1.8 |
There was a problem hiding this comment.
Actually - rethinking this - maybe it's good to add this as a pre-commit step that also fixes this. WDYT?
There was a problem hiding this comment.
Good idea — a pre-commit hook that auto-strips the name fields would catch this earlier and fix it automatically instead of just failing. I'll add it as a pre-commit hook and keep the release validation as a safety net.
There was a problem hiding this comment.
I think we can also remove the release validation, not sure it's a must
There was a problem hiding this comment.
Done — removed the release validation step. The pre-commit hook now handles it by auto-stripping the name fields.
The hook runs on package-lock.yml changes, strips any 'name' fields (unsupported in dbt 1.8), and exits 1 so the user re-stages the fix. Placed before the existing verify-dbt-project-packages-lock hook. Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
The pre-commit hook handles this automatically now, so the release validation step is redundant. Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
Summary
Fixes the
dbt depsfailure inside the Docker image caused by thepackage-lock.ymlcontainingnamefields that are unsupported in dbt 1.8. Also removesdbt-verticafrom theallextra (it pinsdbt-core==1.8.5, blocking resolution to newer versions), and adds a release-time validation to prevent the lock file issue from recurring.Changes:
elementary/monitor/dbt_project/package-lock.yml— Removednamefields from package entries. These are unsupported in dbt 1.8 (though supported in older and newer dbt versions).pyproject.toml— Removeddbt-verticafrom theallextra. It pinsdbt-core==1.8.5, forcing the entire resolution to dbt 1.8.dbt-fabricsparkwas already excluded in Fix Docker build: switch to uv and tighten dbt adapter lower bounds to >=1.8 #2157 due to broken upstream deps.dbt-clickhouseanddbt-sqlserverremain inall— their latest versions (1.10.0and1.9.0) requiredbt-core>=1.9and are compatible with newer dbt-core. Added comments above theallline explaining why fabricspark and vertica are excluded..github/workflows/release.yml— Added a validation step that fails the release ifpackage-lock.ymlcontainsnamefields (unsupported in dbt 1.8).Verified locally: Docker image builds and
dbt depssucceeds inside the container. dbt-core resolves to 1.11.6 with dbt-clickhouse 1.10.0 and dbt-sqlserver 1.9.0 all working together.Updates since last revision
dbt-clickhouseanddbt-sqlserverback to theallextra — onlydbt-vertica(exact==1.8.5pin) is actually problematic. The latest versions of clickhouse and sqlserver have open-endeddbt-core>=1.9constraints.namefield is not deprecated — it's generated by newer dbt versions but rejected by dbt 1.8 specifically).pyproject.tomlabove theallextra.Review & Testing Checklist for Human
allextra scope change: Anyone doingpip install elementary-data[all]will no longer getdbt-vertica. Confirm this is acceptable and consider whether it needs a changelog/migration note for v0.23.1.sha1_hashin lock file: The original hash was kept after removingnamefields. dbt accepted it in local testing, but verify this doesn't cause issues in other dbt versions or environments.docker build . && docker run <image> report --help(or a real report command with a warehouse connection) to confirm the full flow works.Notes
package-lock.ymlissue was masked until now because the Docker build itself was failing (first pip resolution-too-deep, then QEMU segfault). Now that the build succeeds, this was the next failure in the chain.validate-packages-ymljob's Python + PyYAML setup.Link to Devin session: https://app.devin.ai/sessions/a67f519882744e45b279502a0fe54bde
Requested by: @haritamar
Summary by CodeRabbit
dbt-verticaanddbt-fabricsparkfrom the "all" extras; these adapters remain available as individual optional dependencies