-
Notifications
You must be signed in to change notification settings - Fork 7
feat: index storage backend (cache default, PR opt-in) #71
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -814,19 +814,59 @@ def update_indexes_if_needed(): | |
| return updated_folders | ||
|
|
||
|
|
||
| _CACHE_MANIFEST_PATH = "/tmp/code-to-docs-index-cache" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [medium] undocumented-cache-contract The cache backend saves to /tmp/code-to-docs-index-cache but no documentation shows consumers how to configure the required GitHub Actions cache steps for this path. Suggested fix: Expose the cache path via action.yml outputs, or document the cache workflow configuration in README. |
||
|
|
||
|
|
||
| def save_indexes_to_cache(): | ||
| """Save indexes to a well-known path for Actions cache restore.""" | ||
| docs_root = get_docs_root().resolve() | ||
| index_path = docs_root / INDEX_DIR | ||
| if not index_path.exists(): | ||
| print("No indexes to cache") | ||
| return False | ||
| cache_dir = Path(_CACHE_MANIFEST_PATH) | ||
| if cache_dir.exists(): | ||
| shutil.rmtree(cache_dir) | ||
| shutil.copytree(index_path, cache_dir) | ||
| print(f"Indexes saved to cache path: {_CACHE_MANIFEST_PATH}") | ||
| return True | ||
|
|
||
|
|
||
| def restore_indexes_from_cache(): | ||
| """Restore indexes from the Actions cache path if available.""" | ||
| cache_dir = Path(_CACHE_MANIFEST_PATH) | ||
| if not cache_dir.exists(): | ||
| return False | ||
| docs_root = get_docs_root().resolve() | ||
| index_path = docs_root / INDEX_DIR | ||
| if index_path.exists(): | ||
| shutil.rmtree(index_path) | ||
| shutil.copytree(cache_dir, index_path) | ||
| print(f"Indexes restored from cache ({_CACHE_MANIFEST_PATH})") | ||
| return True | ||
|
|
||
|
|
||
| def commit_indexes_to_repo(content_type="indexes"): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] scope-expansion commit_indexes_to_repo() now handles both storage routing and PR-based persistence. Consider extracting backends into separate functions. |
||
| """ | ||
| Commit the .doc-index folder and open a PR to the base branch. | ||
| Persist the .doc-index folder using the configured storage backend. | ||
|
|
||
| Instead of pushing directly to main (which bypasses branch protection), | ||
| this pushes to a persistent branch and creates/updates a PR. | ||
| The INDEX_STORAGE env var controls the backend: | ||
| - "pr" (default): push to a branch and open a PR | ||
| - "cache": save to a well-known path for Actions cache | ||
| - "none": skip persistence | ||
|
|
||
| Args: | ||
| content_type: What's being committed - "indexes", "summaries", or both | ||
|
|
||
| Returns: | ||
| bool: True if content was committed and PR created/updated, False otherwise | ||
| bool: True if content was persisted, False otherwise | ||
| """ | ||
| storage = os.environ.get("INDEX_STORAGE", "pr").lower() | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [medium] default-value-mismatch Python fallback default for INDEX_STORAGE is 'pr' (os.environ.get('INDEX_STORAGE', 'pr')), but action.yml declares the default as 'cache'. Outside the GitHub Action context (local dev, direct script execution), the Python default silently uses PR-based storage, contradicting the documented default. Suggested fix: Change to os.environ.get('INDEX_STORAGE', 'cache'). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [medium] config-centralization INDEX_STORAGE is accessed directly via os.environ.get() instead of through a config.py getter, breaking the codebase's established pattern for testable configuration access. Suggested fix: Add get_index_storage() to config.py. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] missing-validation No validation of INDEX_STORAGE value. An unrecognized value silently falls through to the PR-creation path. |
||
| if storage == "none": | ||
| print(f"Index storage disabled (INDEX_STORAGE=none), skipping {content_type} persistence") | ||
| return False | ||
| if storage == "cache": | ||
| return save_indexes_to_cache() | ||
| docs_root = get_docs_root().resolve() | ||
| index_path = docs_root / INDEX_DIR | ||
|
|
||
|
|
@@ -1244,14 +1284,17 @@ def checkout_docs_from_base_branch(): | |
|
|
||
| def fetch_indexes_from_main(): | ||
| """ | ||
| Fetch indexes and summaries from the main/base branch. | ||
| Fetch indexes and summaries from cache or the main/base branch. | ||
|
|
||
| This ensures PRs can benefit from cached indexes and summaries on main, | ||
| even if they were generated by previous PR runs. | ||
| Tries the Actions cache path first (when INDEX_STORAGE=cache), then | ||
| falls back to fetching from the git branch. | ||
|
|
||
| Returns: | ||
| bool: True if indexes/summaries were fetched, False otherwise | ||
| """ | ||
| if restore_indexes_from_cache(): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [medium] logic-error fetch_indexes_from_main() unconditionally calls restore_indexes_from_cache() regardless of the INDEX_STORAGE setting. When INDEX_STORAGE is 'pr' or 'none', stale cache data from /tmp/code-to-docs-index-cache is loaded if the path exists, skipping the git-based fetch entirely. Suggested fix: Guard with: if os.environ.get('INDEX_STORAGE', 'cache').lower() == 'cache'. |
||
| return True | ||
|
|
||
| docs_root = get_docs_root().resolve() | ||
|
|
||
| # Determine target directory and relative path | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[medium] backward-incompatible
Default value for index-storage changes from implicit 'pr' to explicit 'cache'. Existing consumers relying on the index PR workflow will experience a silent behavior change.
Suggested fix: Keep default as 'pr' for backward compatibility, or document the migration path in README/CHANGELOG.