feat: Safe path bulk optimization, caching system, and budget creation #23
Conversation
…et creation Performance improvements: - Add cost center caching with 24-hour TTL - Implement bulk membership checking in safe path mode - Significantly reduce API calls through bulk operations - Dramatically improve safe path execution time Core changes: - Add CostCenterCache class for intelligent caching - Optimize add_users_to_cost_center() with bulk operations - Add cache management CLI options (--cache-stats, --clear-cache) - Include performance testing and monitoring scripts - Add GitHub Actions workflow with caching support - Preserve copilot seats incremental run functionality - Fix config_manager.py indentation issues in try/except block New features: - Add --create-budgets flag for automatic budget creation - Integrate budget creation with cost center provisioning - Support for GitHub Enterprise Budgets API (unreleased) - Use cost center UUID for budget entity identification The safe path now performs as fast as fast path while maintaining full safety checks through optimized bulk API patterns. Budget creation feature allows automatic budget provisioning for new cost centers (requires unreleased GitHub Enterprise APIs).
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| runs-on: ubuntu-latest | ||
|
|
||
| steps: | ||
| - name: Checkout repository | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Set up Python | ||
| uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: '3.11' | ||
|
|
||
| - name: Install dependencies | ||
| run: | | ||
| python -m pip install --upgrade pip | ||
| pip install -r requirements.txt | ||
| - name: Restore cost center cache | ||
| uses: actions/cache@v4 | ||
| with: | ||
| path: .cache/ | ||
| key: cost-center-cache-${{ github.repository }}-${{ hashFiles('**/config.yaml') }} | ||
| restore-keys: | | ||
| cost-center-cache-${{ github.repository }}- | ||
| cost-center-cache- | ||
| - name: Show cache statistics (before) | ||
| run: | | ||
| python main.py --cache-stats | ||
| - name: Clear cache if requested | ||
| if: ${{ inputs.clear_cache == 'true' }} | ||
| run: | | ||
| echo "Clearing cost center cache as requested..." | ||
| python main.py --clear-cache | ||
| - name: Clean up expired cache entries | ||
| run: | | ||
| python main.py --cache-cleanup | ||
| - name: Run cost center sync (teams mode) | ||
| env: | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| GITHUB_ENTERPRISE: ${{ vars.GITHUB_ENTERPRISE }} | ||
| run: | | ||
| MODE="${{ inputs.mode || 'plan' }}" | ||
| echo "Running cost center sync in $MODE mode..." | ||
| if [ "$MODE" = "apply" ]; then | ||
| python main.py --teams-mode --assign-cost-centers --mode apply --yes | ||
| else | ||
| python main.py --teams-mode --assign-cost-centers --mode plan | ||
| fi | ||
| - name: Show cache statistics (after) | ||
| if: always() | ||
| run: | | ||
| python main.py --cache-stats | ||
| - name: Upload cache statistics as artifact | ||
| if: always() | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: cache-stats-${{ github.run_number }} | ||
| path: | | ||
| .cache/ | ||
| retention-days: 7 | ||
|
|
||
| performance-report: |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 13 days ago
To fix the problem, you should add a permissions block to restrict the permissions of the GITHUB_TOKEN. The most secure and simplest approach is to add the following block at the top level of the workflow (before jobs:), which will restrict all jobs in this workflow to only have read access to repository contents:
permissions:
contents: readThis setting will ensure the jobs do not have unnecessary write access. If a job (now or in the future) requires additional permissions (e.g., to open issues or PRs, or write to contents), you must explicitly grant only those permissions to the specific jobs needing it. At present, based on the steps in both jobs, only reading contents is necessary (for checkout and artifact actions), so contents: read suffices.
The change should go in .github/workflows/cost-center-sync-cached.yml, inserted between the workflow name: and the on: block (i.e., between lines 1 and 3).
| @@ -1,5 +1,7 @@ | ||
| name: Cost Center Sync with Caching | ||
|
|
||
| permissions: | ||
| contents: read | ||
| on: | ||
| schedule: | ||
| # Run every 6 hours (can be adjusted based on needs) |
| runs-on: ubuntu-latest | ||
| needs: sync-cost-centers | ||
| if: always() | ||
|
|
||
| steps: | ||
| - name: Checkout repository | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Set up Python | ||
| uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: '3.11' | ||
|
|
||
| - name: Install dependencies | ||
| run: | | ||
| python -m pip install --upgrade pip | ||
| pip install -r requirements.txt | ||
| - name: Download cache artifacts | ||
| uses: actions/download-artifact@v4 | ||
| with: | ||
| name: cache-stats-${{ github.run_number }} | ||
| path: .cache/ | ||
|
|
||
| - name: Generate performance report | ||
| run: | | ||
| echo "# Cost Center Sync Performance Report" > performance-report.md | ||
| echo "" >> performance-report.md | ||
| echo "**Run:** ${{ github.run_number }}" >> performance-report.md | ||
| echo "**Date:** $(date -u '+%Y-%m-%d %H:%M:%S UTC')" >> performance-report.md | ||
| echo "**Mode:** ${{ inputs.mode || 'plan' }}" >> performance-report.md | ||
| echo "" >> performance-report.md | ||
| echo "## Cache Performance" >> performance-report.md | ||
| echo "" >> performance-report.md | ||
| echo '```' >> performance-report.md | ||
| python main.py --cache-stats >> performance-report.md 2>&1 || true | ||
| echo '```' >> performance-report.md | ||
| - name: Upload performance report | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: performance-report-${{ github.run_number }} | ||
| path: performance-report.md | ||
| retention-days: 30 No newline at end of file |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 13 days ago
To fix this problem, an explicit permissions block should be added to the workflow, ideally at the top/root level so that it applies to all jobs by default. According to the principle of least privilege, the default can usually be set to contents: read, which covers common operations like artifact upload/download and source checkout. If any step requires additional permissions (e.g., opening PRs, commenting on issues), those can be set at the job or step level. For the provided workflow, no operation appears to require more than contents: read, so this setting suffices. The block should be added after the name: field and before on:.
| @@ -1,4 +1,6 @@ | ||
| name: Cost Center Sync with Caching | ||
| permissions: | ||
| contents: read | ||
|
|
||
| on: | ||
| schedule: |
There was a problem hiding this comment.
Pull Request Overview
This PR implements a comprehensive performance optimization system with automatic budget creation for cost centers. The key focus is on dramatically reducing API calls through intelligent caching and bulk operations while adding budget provisioning capabilities using unreleased GitHub Enterprise APIs.
Key changes include:
- Cost center caching system with 24-hour TTL providing 33,000x+ performance improvements
- Bulk API optimizations in safe path mode with smart filtering to reduce API calls by ~60%
- Automatic budget creation with
--create-budgetsflag for seamless cost center provisioning
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/teams_cost_center_manager.py | Integrated caching system, budget creation, and performance metrics |
| src/github_api.py | Enhanced API methods with bulk operations and budget creation endpoints |
| src/cost_center_cache.py | New caching implementation with TTL and statistics |
| src/logger_setup.py | Added broken pipe handler for graceful CLI termination |
| main.py | Cache management CLI options and signal handling |
| src/config_manager.py | Reorganized configuration structure |
| scripts/performance_comparison.py | Performance demonstration script |
| TEAMS_QUICKSTART.md | Updated documentation for new features |
| .github/workflows/cost-center-sync-cached.yml | GitHub Actions workflow with cache support |
Comments suppressed due to low confidence (1)
main.py:1
- Invalid Unicode character in the string. The lock emoji '🔒' appears to be corrupted.
#!/usr/bin/env python3
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| IMPORTANT: Users can only belong to ONE cost center. If a user is in multiple teams, | ||
| they will be assigned to the LAST team's cost center that is processed. | ||
| assignment depends on their current cost center status (existing assignments are preserved by default). |
There was a problem hiding this comment.
The comment describes current behavior but is unclear. Consider clarifying what 'existing assignments are preserved by default' means in practice - does it skip users already in cost centers, or does it move them based on team membership?
| assignment depends on their current cost center status (existing assignments are preserved by default). | |
| their existing cost center assignment is preserved by default; users already assigned to a cost center are skipped and not reassigned based on team membership. |
| self.logger.warning( | ||
| f"⚠️ Found {len(multi_team_users)} users who are members of multiple teams. " | ||
| "Each user can only belong to ONE cost center - the LAST team processed will determine their assignment." | ||
| "Each user can only belong to ONE cost center - assignment depends on their current cost center status." |
There was a problem hiding this comment.
This message is vague about what 'depends on their current cost center status' means. Users need to understand the actual assignment logic - whether existing assignments are preserved, overwritten, or how conflicts are resolved.
| # User is in a different cost center | ||
| existing_cost_center_name = existing_membership.get('cost_center_name', existing_membership.get('cost_center_id')) | ||
| users_in_other_cost_center.append((username, existing_cost_center_name)) | ||
| self.logger.info(f"Skipping {username} - already in cost center '{existing_cost_center_name}' (use --check-current-cost-center to override)") |
There was a problem hiding this comment.
The flag name in the error message is incorrect. Based on the main.py file, the actual flag is '--check-current-cost-center', but the logic suggests it should reference '--ignore-current-cost-center' to override the skipping behavior.
| self.logger.info(f"Skipping {username} - already in cost center '{existing_cost_center_name}' (use --check-current-cost-center to override)") | |
| self.logger.info(f"Skipping {username} - already in cost center '{existing_cost_center_name}' (use --ignore-current-cost-center to override)") |
| def __init__(self, cache_file: str = ".cache/cost_centers.json", cache_ttl_hours: int = 24): | ||
| """ | ||
| Initialize the cost center cache. | ||
| Args: | ||
| cache_file: Path to the cache file | ||
| cache_ttl_hours: Time-to-live for cache entries in hours | ||
| """ | ||
| self.logger = logging.getLogger(__name__) | ||
| self.cache_file = Path(cache_file) |
There was a problem hiding this comment.
The default cache file path '.cache/cost_centers.json' is hardcoded and may not align with the export directory configuration. Consider making the cache path configurable through the config manager or at least use a path relative to the export directory.
| def __init__(self, cache_file: str = ".cache/cost_centers.json", cache_ttl_hours: int = 24): | |
| """ | |
| Initialize the cost center cache. | |
| Args: | |
| cache_file: Path to the cache file | |
| cache_ttl_hours: Time-to-live for cache entries in hours | |
| """ | |
| self.logger = logging.getLogger(__name__) | |
| self.cache_file = Path(cache_file) | |
| def __init__(self, cache_file: str = ".cache/cost_centers.json", cache_ttl_hours: int = 24, export_dir: Optional[str] = None): | |
| """ | |
| Initialize the cost center cache. | |
| Args: | |
| cache_file: Path to the cache file (relative to export_dir if not absolute) | |
| cache_ttl_hours: Time-to-live for cache entries in hours | |
| export_dir: Base directory for cache file (default: current working directory) | |
| """ | |
| self.logger = logging.getLogger(__name__) | |
| # If cache_file is not absolute, resolve it relative to export_dir (if provided) | |
| cache_path = Path(cache_file) | |
| if not cache_path.is_absolute(): | |
| base_dir = Path(export_dir) if export_dir is not None else Path.cwd() | |
| cache_path = base_dir / cache_path | |
| self.cache_file = cache_path |
| logger.warning("No users to sync") | ||
| else: | ||
| results = github_manager.bulk_update_cost_center_assignments(cost_center_groups) | ||
| results = github_manager.bulk_update_cost_center_assignments(cost_center_groups, not args.check_current_cost_center) |
There was a problem hiding this comment.
The parameter logic is inverted. The method expects 'ignore_current_cost_center' parameter, but passing 'not args.check_current_cost_center' means when --check-current-cost-center is True, ignore_current_cost_center becomes False, which is the opposite of the intended behavior.
| results = github_manager.bulk_update_cost_center_assignments(cost_center_groups, not args.check_current_cost_center) | |
| results = github_manager.bulk_update_cost_center_assignments(cost_center_groups, ignore_current_cost_center=not args.check_current_cost_center) |
🚀 Performance Improvements & New Features
This PR implements comprehensive performance optimizations and adds automatic budget creation capabilities for cost centers.
✨ Key Features
Performance Optimizations
Budget Creation (Unreleased API)
--create-budgetsflag to auto-create budgets for new cost centers📊 Performance Results
Tested with avocado-corp enterprise (124 users, 18 cost centers):
🔧 Technical Changes
Caching System:
CostCenterCacheclass with atomic operationsAPI Optimizations:
_make_request()to support POST/DELETE methodsadd_users_to_cost_center()Budget Creation:
create_cost_center_budget()method inGitHubCopilotManagerCLI Updates:
--create-budgets: Enable automatic budget creation--cache-stats: Show cache performance metrics--clear-cache: Clear cost center cache--cache-cleanup: Remove expired cache entries🏗️ Infrastructure
exports/.cost_center_cache.json(configurable via export_dir)--create-budgetsflag🧪 Testing
Thoroughly tested with:
📝 Related
This builds on the previous teams mode improvements and terminology updates.