Docs/issue 1337 changelog script #1361
Conversation
|
Hi, this is MergeConflictBot. Please resolve these conflicts locally and push the changes. To assist you, please read: Thank you for contributing! |
|
Hi, this is WorkflowBot.
|
📝 WalkthroughWalkthroughA new Bash script validates changelog discipline in PRs by comparing CHANGELOG.md against upstream main, extracting added/deleted entries, and validating correct placement under Unreleased sections with appropriate subtitles. The CHANGELOG.md is updated to document this new script. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/scripts/pr-check-changelog.sh (2)
78-90: Validate required environment variables and handle git command failures.Per coding guidelines, scripts MUST validate all required environment variables. Additionally, the git commands need proper error handling:
GITHUB_REPOSITORYis used without validationgit remote add upstreamwill fail if the remote already existsgit fetcherrors are suppressed, allowing the script to continue with stale/missing data- The
CHANGELOGvariable should be readonly to prevent accidental modification🔎 Proposed fix
-CHANGELOG="CHANGELOG.md" +readonly CHANGELOG="CHANGELOG.md" + +# Validate required environment variables +if [[ -z "${GITHUB_REPOSITORY:-}" ]]; then + echo -e "${RED}❌ Error: GITHUB_REPOSITORY environment variable is not set${RESET}" + exit 1 +fi + +# Validate CHANGELOG.md exists +if [[ ! -f "$CHANGELOG" ]]; then + echo -e "${RED}❌ Error: $CHANGELOG file not found${RESET}" + exit 1 +fi # ANSI color codes RED="\033[31m" GREEN="\033[32m" YELLOW="\033[33m" RESET="\033[0m" failed=0 # Fetch upstream -git remote add upstream https://github.com/${GITHUB_REPOSITORY}.git -git fetch upstream main >/dev/null 2>&1 +git remote add upstream "https://github.com/${GITHUB_REPOSITORY}.git" 2>/dev/null || true +if ! git fetch upstream main 2>&1; then + echo -e "${RED}❌ Error: Failed to fetch upstream/main${RESET}" + exit 1 +fiBased on coding guidelines, scripts must validate all required environment variables and handle command failures explicitly.
108-118: Consider adding defensive checks for empty CHANGELOG.The diff extraction logic is solid, but if
CHANGELOG.mdis empty or the diff fails, the script should detect this earlier. The current logic will continue processing even ifraw_diffis empty, potentially causing confusing error messages.🔎 Proposed enhancement
# Get raw diff raw_diff=$(git diff upstream/main -- "$CHANGELOG") + +# Check if diff could be obtained +if [[ -z "$raw_diff" ]]; then + echo -e "${GREEN}✅ No changes to $CHANGELOG detected.${RESET}" + exit 0 +fi # 1️⃣ Show raw diff with colors
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.github/scripts/pr-check-changelog.shCHANGELOG.md
🧰 Additional context used
📓 Path-based instructions (1)
.github/scripts/**/*.sh
⚙️ CodeRabbit configuration file
.github/scripts/**/*.sh: Treat shell scripts as production-grade automation.Scripts should be small, focused, and explicit.
Avoid “do-everything” or overly generic scripts.
- MUST use:
set -euo pipefail- MUST validate all required environment variables
- MUST defensively quote variables
- MUST validate all untrusted input
- MUST have bounded loops and pagination
- MUST support dry-run mode if state is mutated
- MUST log key decisions and early exits
Files:
.github/scripts/pr-check-changelog.sh
🪛 LanguageTool
CHANGELOG.md
[uncategorized] ~86-~86: The official name of this software platform is spelled with a capital “H”.
Context: ...tation to the PR changelog check script (.github/scripts/pr-check-changelog.sh) to clar...
(GITHUB)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (4)
CHANGELOG.md (1)
86-87: LGTM! Changelog entries are well-documented.The entries clearly describe the documentation additions and script renaming, with proper issue references and placement under the Changed section.
.github/scripts/pr-check-changelog.sh (3)
3-76: Excellent comprehensive documentation!The docstring is exceptionally thorough and well-structured. It clearly explains:
- When and why the script executes
- The state machine concept (Safe Zone vs Danger Zone)
- The 5-step validation flow
- All requirements, dependencies, and permissions
This level of documentation significantly improves maintainability and aligns perfectly with the PR objectives.
127-166: LGTM! State machine logic correctly implements the documented validation flow.The classification logic properly tracks context (release section, subtitle, and unreleased flag) and correctly categorizes entries into the three buckets as documented. The implementation matches the detailed flow description in the docstring.
186-193: Verify: Should deleted changelog entries fail the check?The script currently displays deleted entries as warnings (yellow) but doesn't set
failed=1. This means PRs that remove changelog entries will pass validation.Is this intentional? In most changelog workflows, removing entries from Unreleased would be acceptable (e.g., when moving them to a new release section), but removing entries from released versions should likely fail the check.
Consider whether the logic should be:
- Deletions from
[Unreleased]: Warning only (current behavior)- Deletions from released versions: Fail the check
If you'd like to enforce stricter validation, I can provide an implementation that tracks where deletions occurred and fails appropriately.
| #!/bin/bash | ||
|
|
There was a problem hiding this comment.
Add required error handling directives.
Per coding guidelines, scripts in .github/scripts/**/*.sh MUST use set -euo pipefail to fail fast on errors, undefined variables, and pipeline failures.
🔎 Proposed fix
#!/bin/bash
+set -euo pipefail
Based on coding guidelines, all scripts in .github/scripts/ must include this directive.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #!/bin/bash | |
| #!/bin/bash | |
| set -euo pipefail | |
1 participant
Description
The goal was to document the script's functionality, logic flow, and requirements without affecting its execution.
Technical Overview
After researching and analyzing the script's logic, I have documented:
Key Documentation Points
Note: This docstring was crafted based on a manual review of the script logic to ensure it serves as a helpful guide for future maintainers.