- Notifications
You must be signed in to change notification settings - Fork 2
Fix overlay detection and remove duplication #33
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
Conversation
| Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis pull request introduces four new helper scripts to support admin API versioning and overlay management in the documentation build process. The Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
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.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
.github/scripts/detect-admin-versions.sh(1 hunks).github/scripts/determine-overlays.sh(1 hunks).github/scripts/test-overlays.sh(1 hunks).github/scripts/validate-workflow.sh(1 hunks).github/workflows/bump.yml(6 hunks)
🧰 Additional context used
🪛 actionlint (1.7.8)
.github/workflows/bump.yml
94-94: shellcheck reported issue in this script: SC2086:info:2:35: Double quote to prevent globbing and word splitting
(shellcheck)
102-102: shellcheck reported issue in this script: SC2086:info:2:35: Double quote to prevent globbing and word splitting
(shellcheck)
110-110: shellcheck reported issue in this script: SC2086:info:2:35: Double quote to prevent globbing and word splitting
(shellcheck)
199-199: shellcheck reported issue in this script: SC2086:info:2:35: Double quote to prevent globbing and word splitting
(shellcheck)
207-207: shellcheck reported issue in this script: SC2086:info:2:35: Double quote to prevent globbing and word splitting
(shellcheck)
215-215: shellcheck reported issue in this script: SC2086:info:2:35: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (7)
.github/scripts/test-overlays.sh (1)
1-94: Test script is well-structured and comprehensive. The overlay detection tests correctly verify file existence, and edge case handling (empty doc_id should error, admin without version should return empty) aligns with determine-overlays.sh behavior. The admin version detection test properly parses JSON output via jq..github/scripts/detect-admin-versions.sh (1)
1-31: Version detection and JSON output are correct. The script properly handles the v1 base case and extracts versions from versioned files using filename parsing. The nullglob safety and jq array composition are well-executed..github/scripts/determine-overlays.sh (1)
1-67: Overlay determination logic is sound. The script correctly branches on admin vs non-admin, handles versioning, and uses the comma-separated list format consistently. The prefix-based routing for cloud vs self-managed docs is a clean approach..github/scripts/validate-workflow.sh (1)
1-133: Workflow validation script is comprehensive for basic checks. The tests cover YAML syntax, script availability, step IDs, output references, conditionals, and AWS credential placement. The grep-based validation is somewhat brittle (e.g., grep patterns for conditionals at lines 79, 87 may miss variations in quoting or formatting), but appropriate for a CI validation script..github/workflows/bump.yml (3)
60-69: AWS credentials correctly moved into individual jobs. The placement in deploy-doc and api-diff jobs (rather than determine-doc-ids) aligns with the principle of least privilege and matches the PR objective to limit credentials to only the jobs that need them. This also removes unnecessary credential overhead from the matrix-determining job.Also applies to: 165-174
77-77: Admin vs non-admin branching via conditionals is clear and correct. The use ofif: ${{ matrix.doc_id != 'admin' }}andif: ${{ matrix.doc_id == 'admin' }}cleanly separates overlay determination and deployment for admin v1, admin v2, and non-admin documentation. The pattern is consistently applied across both deploy-doc and api-diff jobs.Also applies to: 93-93, 101-101, 109-109, 145-145, 182-182, 198-198, 206-206, 252-252
99-114: Overlay determination steps correctly delegate to helper scripts. Each branch invokes determine-overlays.sh with the appropriate doc_id and optional admin-version parameter, stores the result in outputs, and feeds it to the bump-sh action. The separation of admin v1, admin v2, and non-admin overlays improves clarity and maintainability.Please verify that the following are in place before merging:
Script executability: Confirm
.github/scripts/determine-overlays.sh,detect-admin-versions.sh,test-overlays.sh, andvalidate-workflow.share marked as executable (mode 755) in the repository, otherwise the workflow will fail with "Permission denied" errors.Overlay output format compatibility: The scripts output comma-separated file paths (e.g.,
admin/v1-overlays/foo.yaml,shared/overlays/sm-bar.yaml). Verify that the bump-sh GitHub Action (bump-sh/github-action@v1) correctly interprets this format for theoverlay:parameter. Check the action's documentation to confirm it expects comma-separated paths or if a different delimiter is required.Script availability at runtime: Ensure that
.github/scripts/determine-overlays.shis present and executable when the workflow runs (the scripts must be committed to the repository and the git checkout in the workflow must fetch them).Also applies to: 204-219
This pull request refactors the GitHub Actions workflow for API documentation deployment and diffing, focusing on modularity and maintainability. Overlay detection logic has been moved out of the workflow YAML into dedicated shell scripts, and admin API version detection is now handled by a script. The workflow steps for overlays and admin version handling are simplified and made more robust, with additional validation and testing scripts included. AWS credential usage is clarified and limited to appropriate jobs.
Workflow and Script Refactoring
.github/workflows/bump.ymlto a new script.github/scripts/determine-overlays.sh, which dynamically determines overlays for both admin and non-admin docs. Workflow steps now call this script instead of duplicating logic. [1] [2] [3].github/scripts/detect-admin-versions.shto automatically detect available admin API versions and output them as JSON. This is now called in the workflow to set up admin version branches and overlays. [1] [2] [3]steps.admin-v1-overlays.outputs.overlay_pathsandsteps.admin-v2-overlays.outputs.overlay_paths), rather than the generic overlays step. [1] [2]Testing and Validation
.github/scripts/test-overlays.shto test overlay detection for various document types and edge cases, ensuring overlays are correctly found and errors are handled..github/scripts/validate-workflow.shto validate the workflow file for YAML syntax, required script existence, correct step output references, conditional logic, and AWS credential usage.AWS Credentials Usage
determine-doc-idsjob, restricting AWS usage to deploy and diff jobs only, in line with best practices and workflow validation. [1] [2]These changes make the workflow easier to maintain and extend, reduce duplication, and improve error handling and validation.