Repository navigation
fix(wren): load cubes from folder-per-entity layout - #2350
Conversation
github-actions
Bot
added
documentation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughCube loading now dispatches on project schema version: v1 reads flat ChangesSchema-versioned cube layout support
🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
It seems to be a bug in the cube loader 🤔 . The layout described by the skill or the document is what we actually expected. |
Got it, thanks for clarifying. I’ll update this PR to fix the cube loader instead, so it recursively discovers cube files in nested directories as expected. |
6e1fdbd to
0e3cfb6
Compare
ttw225
changed the title
|
Updated the PR accordingly. The cube loader now follows the schema-version layout split: I also updated the tests to cover v1 flat cubes, v2 nested cube metadata, and v3 reusing the v2 layout. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@core/wren/src/wren/context.py`:
- Around line 617-637: The upgrade currently changes schema layout so
load_cubes() will call _load_cubes_v2() and ignore legacy flat cube files,
causing silent loss; update the v1→v2 migration by adding cube conversion logic
in _plan_v1_to_v2() / _apply_v1_to_v2() that detects files matching the old
pattern (cubes/*.yml), reads each legacy cube, creates a directory per cube name
under cubes/, writes metadata.yml with the same content (preserving the
_source_file if needed), and removes or archives the original flat file;
alternatively, add a clear warning in the upgrade plan if you prefer manual
migration so users aren’t silently dropped.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 63e98273-0588-4376-907d-a6ce9b2ce477
📒 Files selected for processing (2)
core/wren/src/wren/context.pycore/wren/tests/unit/test_context.py
Migrate legacy cubes/*.yml files into cubes/<name>/metadata.yml during v1 to v2 upgrades so cubes remain visible after the schema_version bump. Update upgrade tests to cover cube migration in the planned and applied upgrade paths.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/wren/tests/unit/test_context.py (1)
944-976:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the documented hierarchy YAML shape in these cube tests.
The public cube contract is
hierarchies: - name: ... levels: [...], but these fixtures use a mapping (hierarchies: drill: [...]).validate_project()currently only inspects dicts, so these tests don't cover the documentedmetadata.ymlform and won't catch the case users actually write. Please switch the fixtures to the list-of-objects shape and normalize that form in validation, or update the docs if the mapping syntax is the real API.🤖 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 `@core/wren/tests/unit/test_context.py` around lines 944 - 976, The tests test_validate_cube_non_string_hierarchy_level and test_validate_cube_bad_hierarchy use the non-documented mapping form for hierarchies; update their _write_cube fixtures to use the public shape "hierarchies: - name: <name> levels: [...]" (e.g. replace "hierarchies:\n drill: ..." or "hierarchies:\n drill:\n - ..." with a list containing an object with name and levels) so validate_project() exercises the documented list-of-objects form; additionally ensure validate_project (the validation logic) normalizes/accepts that list form (or add normalization) so hierarchy validation checks still run against the new shape.
🤖 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 `@core/wren/src/wren/context.py`:
- Around line 1201-1211: Detect and fail on duplicate migration target paths
when planning or applying v1→v2 cube migrations: in the planning phase where
cubes = _load_cubes_v1(project_path) is iterated (the block that builds created
and deleted lists using source_file, name and dir_path) and in the actual
migration implementation _apply_v1_to_v2(), check for collisions on the computed
destination path ("cubes/{name}/metadata.yml") by maintaining a set of seen
target paths; if a duplicate is detected raise UpgradeError with a clear message
instead of adding to created/deleted or writing files. Ensure both planning and
apply use the same helper logic to compute the destination and perform the
collision check so apply_upgrade() cannot silently overwrite and drop a cube.
---
Outside diff comments:
In `@core/wren/tests/unit/test_context.py`:
- Around line 944-976: The tests test_validate_cube_non_string_hierarchy_level
and test_validate_cube_bad_hierarchy use the non-documented mapping form for
hierarchies; update their _write_cube fixtures to use the public shape
"hierarchies: - name: <name> levels: [...]" (e.g. replace "hierarchies:\n
drill: ..." or "hierarchies:\n drill:\n - ..." with a list containing an
object with name and levels) so validate_project() exercises the documented
list-of-objects form; additionally ensure validate_project (the validation
logic) normalizes/accepts that list form (or add normalization) so hierarchy
validation checks still run against the new shape.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8b6c24ef-3a7a-4614-a8ab-ef2136e93da0
📒 Files selected for processing (2)
core/wren/src/wren/context.pycore/wren/tests/unit/test_context.py
Load v1 cubes from legacy cubes/*.yml and v2+ cubes from cubes/<name>/metadata.yml, matching the folder-per-entity project layout. Migrate legacy cube files during v1->v2 upgrades and reject duplicate cube migration targets to avoid silent overwrites. Update cube and upgrade tests for v1, v2, and v3 layout behavior.
Summary
cubes/<name>/metadata.ymlschema_version: 1cube loading fromcubes/*.ymlTesting
uv run pytest tests/unit/test_context.py -k cubeuv run pytest tests/unit/test_context.pySummary by CodeRabbit