Conversation
DTLRepo always ran a clone or a fetch, even when REPO_PATH already held the library contents locally. validate_git_url() rejected the old 'local' value outright, and even a valid URL still triggered a real git fetch against the remote on every run. REPO_URL=local now skips Repo(), clone_from(), and fetch() entirely. REPO_PATH is used as-is and must already contain device-types/, module-types/, and rack-types/ (no .git required, and none is used).
… export (#126) * fix(repo): validate the local library layout and share the check with export REPO_URL=local only checked that REPO_PATH was a directory. An unrelated or empty path passed that check, and discover_vendors skips type directories that are absent, so the run imported nothing and still exited 0. Export mode already had the check it needed, in Exporter._verify_repo_available. Both sides now read one definition of what makes a checkout a library: LIBRARY_TYPE_DIRS and library_dirs_present() in core/repo.py. The sentinel value moves to LOCAL_REPO_URL in core/config.py, beside the other REPO_* defaults, so config and repo cannot drift on it. Local mode still skips validate_repo_path on purpose: that check demands write access, which a read-only or air-gapped mount cannot give, and no import step writes to REPO_PATH. A test pins the read-only case. REPO_BRANCH is ignored under the sentinel, so a run that sets both now says so through the existing config notice mechanism instead of looking like it checked the branch out. Documents the mode in the README and .env.example, including the read-only Docker mount, which is the case that motivated the sentinel. * fix(import): treat an absent type root as empty instead of crashing The layout check accepts any one of device-types/, module-types/, and rack-types/, matching what export mode already accepted. plan_vendor then called get_devices() on all three regardless, and get_devices() lists the directory, so a local checkout holding only some of them raised FileNotFoundError before importing the types it did hold. A device-types-only library is the likeliest local layout, and it crashed on module-types. _parse_vendor_racks already had the guard this needed. It is now _parse_vendor_files and all three roots go through it, so the guard cannot apply to one root and not the others again. The repo mock in test_nb_dt_import pointed at /tmp/devices, /tmp/modules and /tmp/rack-types, paths that never existed. Nothing stat'd them, so the mock passed for a filesystem that was not there, which is why this went unseen. It now points at a real empty library tree, and the tests that matched on those literal paths match on the directory names instead. Found by CodeRabbit on #126.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change adds ChangesLocal repository mode
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change adds a local repository sentinel that bypasses Git while preserving repository availability validation; no actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant Configuration
participant DTLRepo
participant LocalLibrary
participant VendorPlanner
Configuration->>DTLRepo: Pass local URL and REPO_PATH
DTLRepo->>LocalLibrary: Validate supported library directory
DTLRepo->>LocalLibrary: Read files without Git operations
VendorPlanner->>LocalLibrary: Discover available type roots
LocalLibrary-->>VendorPlanner: Return vendor files
VendorPlanner-->>Configuration: Produce type plans
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 57.45% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 47 functions across 9 files. (2 skipped: 2 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
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
2 participants
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.
Summary by CodeRabbit
New Features
Documentation
Tests