Conversation
![hivel-marco[bot]](https://avatars.githubusercontent.com/in/2836183?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Complexity Score: 2.0 - Trivial
View Breakdown
- Lines Changed: 217
- Files Changed: 5
- Complexity Added: 0
- Raw Score: 19.34
⚠️ Sensitive Data Detected
| File | Types | Count | ||||||
|---|---|---|---|---|---|---|---|---|
|
| Line | Type | Preview |
|---|---|---|
| 13 | Secret: Secret Keyword | [Secret Keyword] |
- Introduces a standardized pull request template to improve PR metadata and consistency.
- Adds a script and documentation to generate changelogs from merged PRs via the GitHub API.
- Configures a reusable PR lint workflow for main/master-targeted PRs.
- Adds an automated PR size check workflow with support for exception labels and approval gating.
High-level summary
This PR sets up foundational GitHub workflow and tooling for this repository:
- Standardizes how contributors describe changes (changelog, summary, impact, type, docs).
- Enables automated generation of changelogs based on merged PRs, excluding sync/bot PRs.
- Enforces common PR lint checks for PRs targeting main/master via a shared workflow.
- Introduces a PR size enforcement mechanism on dev/develop branches, including:
- Size thresholds and path exclusions.
- A bypass label (
pr-size-exception) that triggers additional approval requirements and automated comments.
Key areas affected: .github configuration (PR templates, scripts, and workflows).
File-level change summary
| File | Change summary |
|---|---|
.github/pull_request_template.md |
Adds a new PR template capturing changelog, summary, automation status, test report URL, areas of impact, type of change (bugfix/feature/enhancement/tests/docs/chore), and documentation links. |
.github/scripts/README.md |
Documents how to use the new generate-changelog.sh script, including prerequisites, environment variables, arguments, and example commands for different branches/date filters. |
.github/scripts/generate-changelog.sh |
New Bash script that queries the GitHub Search API for merged PRs into a given branch within a date range, filters out parent-branch-sync and bot PRs, handles JSON/HTTP errors robustly, prints a formatted changelog list, and outputs a verification search URL. |
.github/workflows/pr-lint.yml |
Adds a reusable workflow that runs “Common PR Lint Checks” on PR events targeting main or master, delegating to chargebee/cb-cicd-pipelines and inheriting secrets. |
.github/workflows/pr-size-check.yml |
Adds a PR size check workflow for PRs into dev/develop/**, including: a pre-approval comment job when pr-size-exception label is present; a size-check job using a shared action with thresholds (warning 200, error 250) and excluded paths; and a bypass path that marks the job successful while requiring CAB approvals. |
| ## DOCUMENTATION | ||
| REPLACE_ME_WITH_DOCUMENTATION_LINK_OR_NA No newline at end of file |
There was a problem hiding this comment.
Priority: 🟢 LOW
Problem: The file is missing a trailing newline (\n), as indicated by \ No newline at end of file.
Why: Many tools and linters expect text files to end with a newline; missing it can cause unnecessary diffs, minor tooling issues, or violate common repository style conventions.
How to Fix: Add a newline at the end of the file so the last line is properly terminated.
| ## DOCUMENTATION | |
| REPLACE_ME_WITH_DOCUMENTATION_LINK_OR_NA | |
| ## DOCUMENTATION | |
| REPLACE_ME_WITH_DOCUMENTATION_LINK_OR_NA |
| # Optional: Date filter (defaults to last 30 days if not provided) | ||
| DATE_FILTER="${2:-merged:>=$(date -u -v-30d +%Y-%m-%d 2>/dev/null || date -u -d '30 days ago' +%Y-%m-%d)}" | ||
|
|
||
| # Repo is set per-repo when this file is pushed (placeholder replaced by upload script) | ||
| REPO="chargebee/chargebee-react-native" | ||
|
|
||
| echo "🔍 Searching for PRs merged into $SOURCE_BRANCH..." |
There was a problem hiding this comment.
Priority: 🟡 MEDIUM
Problem: The REPO variable is hardcoded to chargebee/chargebee-react-native, which makes this script non-reusable across repositories and contradicts the comment that it will be set per-repo when pushed.
Why: If this script is copied to another repository without updating REPO, it will silently query and generate changelogs for the wrong repo, leading to incorrect release notes and confusion; the misleading comment also increases the risk of misconfiguration.
How to Fix: Either derive REPO dynamically from git remote or require it via an environment variable (with a clear error if missing), and update the comment accordingly so the behavior is explicit and repo-agnostic.
| # Optional: Date filter (defaults to last 30 days if not provided) | |
| DATE_FILTER="${2:-merged:>=$(date -u -v-30d +%Y-%m-%d 2>/dev/null || date -u -d '30 days ago' +%Y-%m-%d)}" | |
| # Repo is set per-repo when this file is pushed (placeholder replaced by upload script) | |
| REPO="chargebee/chargebee-react-native" | |
| echo "🔍 Searching for PRs merged into $SOURCE_BRANCH..." | |
| # Optional: Date filter (defaults to last 30 days if not provided) | |
| DATE_FILTER="${2:-merged:>=$(date -u -v-30d +%Y-%m-%d 2>/dev/null || date -u -d '30 days ago' +%Y-%m-%d)}" | |
| # Repository to query (must be provided as OWNER/REPO) | |
| if [[ -z "$REPO" ]]; then | |
| echo "❌ Error: Environment variable REPO (format OWNER/REPO) not found" | |
| exit 1 | |
| fi | |
| echo "🔍 Searching for PRs merged into $SOURCE_BRANCH..." |
| HTTP_STATUS=$(curl -s -w "%{http_code}" -G -u "$GH_USERNAME:$GH_PAT" \ | ||
| "https://api.github.com/search/issues" \ | ||
| --data-urlencode "q=NOT \"Parent branch sync\" in:title is:pr repo:$REPO is:merged base:$SOURCE_BRANCH merged:$DATE_FILTER -author:app/distributed-gitflow-app" \ | ||
| -o /tmp/curl_output.json \ | ||
| 2>/tmp/curl_error.log) | ||
|
|
||
| CURL_EXIT_CODE=$? |
There was a problem hiding this comment.
Priority: 🟡 MEDIUM
Problem: The script writes API responses and errors to fixed paths under /tmp (/tmp/curl_output.json, /tmp/curl_error.log), which can cause collisions when multiple instances run concurrently and may expose sensitive data in shared temp files.
Why: Concurrent executions (e.g., multiple users or CI jobs on the same runner) can overwrite each other’s temp files, leading to incorrect results or confusing diagnostics; additionally, storing API responses with potentially sensitive metadata in predictable global paths increases the risk of unintended access.
How to Fix: Use uniquely named temporary files (e.g., via mktemp) scoped to the current process, and ensure they are cleaned up at the end or on error.
| HTTP_STATUS=$(curl -s -w "%{http_code}" -G -u "$GH_USERNAME:$GH_PAT" \ | |
| "https://api.github.com/search/issues" \ | |
| --data-urlencode "q=NOT \"Parent branch sync\" in:title is:pr repo:$REPO is:merged base:$SOURCE_BRANCH merged:$DATE_FILTER -author:app/distributed-gitflow-app" \ | |
| -o /tmp/curl_output.json \ | |
| 2>/tmp/curl_error.log) | |
| CURL_EXIT_CODE=$? | |
| OUTPUT_FILE="$(mktemp /tmp/changelog_curl_output.XXXXXX.json)" | |
| ERROR_FILE="$(mktemp /tmp/changelog_curl_error.XXXXXX.log)" | |
| HTTP_STATUS=$(curl -s -w "%{http_code}" -G -u "$GH_USERNAME:$GH_PAT" \ | |
| "https://api.github.com/search/issues" \ | |
| --data-urlencode "q=NOT \"Parent branch sync\" in:title is:pr repo:$REPO is:merged base:$SOURCE_BRANCH merged:$DATE_FILTER -author:app/distributed-gitflow-app" \ | |
| -o "$OUTPUT_FILE" \ | |
| 2>"$ERROR_FILE") | |
| CURL_EXIT_CODE=$? |
| if [ $CURL_EXIT_CODE -ne 0 ]; then | ||
| echo "❌ Error: curl request failed with exit code $CURL_EXIT_CODE" | ||
| echo "Error details: $(cat /tmp/curl_error.log)" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Priority: 🟡 MEDIUM
Problem: When curl fails, the script unconditionally runs cat /tmp/curl_error.log, which will itself fail if the file is missing or renamed (e.g., after refactoring to use mktemp), and it doesn’t guard against empty error output.
Why: A secondary failure while printing error details can obscure the original problem and make debugging harder, especially in CI logs where only the last error is visible.
How to Fix: Check for the existence and non-emptiness of the error file before reading it, and fall back to a generic message if it’s not available.
| if [ $CURL_EXIT_CODE -ne 0 ]; then | |
| echo "❌ Error: curl request failed with exit code $CURL_EXIT_CODE" | |
| echo "Error details: $(cat /tmp/curl_error.log)" | |
| exit 1 | |
| fi | |
| if [ $CURL_EXIT_CODE -ne 0 ]; then | |
| echo "❌ Error: curl request failed with exit code $CURL_EXIT_CODE" | |
| if [[ -s "$ERROR_FILE" ]]; then | |
| echo "Error details: $(cat "$ERROR_FILE")" | |
| else | |
| echo "Error details: (no additional error output available)" | |
| fi | |
| exit 1 | |
| fi |
| if [ "$HTTP_STATUS" -ne 200 ]; then | ||
| echo "❌ Error: API returned HTTP status $HTTP_STATUS" | ||
| echo "Response: $(cat /tmp/curl_output.json)" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Priority: 🟡 MEDIUM
Problem: On non-200 HTTP responses, the script prints the entire JSON response inline with $(cat /tmp/curl_output.json), which can be very large and hard to read, and again assumes the fixed temp path exists.
Why: Large unformatted JSON blobs reduce log readability and can make it difficult to spot the actual error message; relying on a hardcoded temp path also breaks if the file naming strategy changes.
How to Fix: Use the same temp variable as for the curl output, and either pretty-print the JSON with jq when available or truncate/indicate where to inspect the full response.
| if [ "$HTTP_STATUS" -ne 200 ]; then | |
| echo "❌ Error: API returned HTTP status $HTTP_STATUS" | |
| echo "Response: $(cat /tmp/curl_output.json)" | |
| exit 1 | |
| fi | |
| if [ "$HTTP_STATUS" -ne 200 ]; then | |
| echo "❌ Error: API returned HTTP status $HTTP_STATUS" | |
| if command -v jq >/dev/null 2>&1; then | |
| echo "Response:" | |
| jq . "$OUTPUT_FILE" || cat "$OUTPUT_FILE" | |
| else | |
| echo "Response: $(cat "$OUTPUT_FILE")" | |
| fi | |
| exit 1 | |
| fi |
| PR_LIST_RESPONSE=$(cat /tmp/curl_output.json) | ||
|
|
||
| # Clean invalid control characters from JSON response |
There was a problem hiding this comment.
Priority: 🟡 MEDIUM
Problem: The script reads the entire API response into a shell variable (PR_LIST_RESPONSE=$(cat /tmp/curl_output.json)), which is unnecessary and can be fragile for large responses.
Why: Storing large JSON payloads in shell variables can hit shell limits and makes the script more memory-intensive; it’s more robust to stream from the file directly into jq and other tools.
How to Fix: Avoid loading the full JSON into a variable; instead, operate directly on the output file with jq and only assign small scalar values (like total_count) to variables.
| PR_LIST_RESPONSE=$(cat /tmp/curl_output.json) | |
| # Clean invalid control characters from JSON response | |
| PR_LIST_RESPONSE_FILE="$OUTPUT_FILE" | |
| # Clean invalid control characters from JSON response |
| # Clean invalid control characters from JSON response | ||
| if ! echo "$PR_LIST_RESPONSE" | jq . >/dev/null 2>&1; then | ||
| echo "⚠️ Invalid JSON detected — cleaning control characters..." | ||
| PR_LIST_RESPONSE=$(echo "$PR_LIST_RESPONSE" | tr -d '\000-\037') | ||
|
|
||
| if ! echo "$PR_LIST_RESPONSE" | jq . >/dev/null 2>&1; then | ||
| echo "$PR_LIST_RESPONSE" > /tmp/invalid_json_debug.json | ||
| echo "❌ Error: JSON is still invalid after cleaning control characters" | ||
| echo "💡 Use 'cat /tmp/invalid_json_debug.json' to inspect the JSON" | ||
| exit 1 | ||
| fi | ||
| fi |
There was a problem hiding this comment.
Priority: 🟡 MEDIUM
Problem: The JSON “cleaning” step uses tr -d '\000-\037', which strips all control characters including legitimate whitespace like newlines and tabs, potentially altering valid JSON content.
Why: Removing all control characters can corrupt otherwise valid JSON (e.g., strings containing tabs or newlines), leading to subtle parsing issues; it’s safer to target only known problematic characters or handle encoding issues differently.
How to Fix: Narrow the cleaning to specific problematic characters (e.g., \000-\010\013\014\016-\037) or, if the issue is rare, fail fast with a clear error and let the user inspect the raw JSON rather than mutating it aggressively.
| # Clean invalid control characters from JSON response | |
| if ! echo "$PR_LIST_RESPONSE" | jq . >/dev/null 2>&1; then | |
| echo "⚠️ Invalid JSON detected — cleaning control characters..." | |
| PR_LIST_RESPONSE=$(echo "$PR_LIST_RESPONSE" | tr -d '\000-\037') | |
| if ! echo "$PR_LIST_RESPONSE" | jq . >/dev/null 2>&1; then | |
| echo "$PR_LIST_RESPONSE" > /tmp/invalid_json_debug.json | |
| echo "❌ Error: JSON is still invalid after cleaning control characters" | |
| echo "💡 Use 'cat /tmp/invalid_json_debug.json' to inspect the JSON" | |
| exit 1 | |
| fi | |
| fi | |
| # Clean invalid control characters from JSON response | |
| if ! jq . "$PR_LIST_RESPONSE_FILE" >/dev/null 2>&1; then | |
| echo "⚠️ Invalid JSON detected — cleaning control characters..." | |
| CLEANED_FILE="$(mktemp /tmp/changelog_cleaned_json.XXXXXX.json)" | |
| tr -d '\000-\010\013\014\016-\037' < "$PR_LIST_RESPONSE_FILE" > "$CLEANED_FILE" | |
| if ! jq . "$CLEANED_FILE" >/dev/null 2>&1; then | |
| cp "$CLEANED_FILE" /tmp/invalid_json_debug.json | |
| echo "❌ Error: JSON is still invalid after cleaning control characters" | |
| echo "💡 Use 'cat /tmp/invalid_json_debug.json' to inspect the JSON" | |
| exit 1 | |
| fi | |
| PR_LIST_RESPONSE_FILE="$CLEANED_FILE" | |
| fi |
| PR_MERGED_COUNT=$(echo "$PR_LIST_RESPONSE" | jq '.total_count') | ||
|
|
||
| # Color codes |
There was a problem hiding this comment.
Priority: 🟡 MEDIUM
Problem: PR_MERGED_COUNT is derived from echo "$PR_LIST_RESPONSE" | jq '.total_count', which assumes jq is installed but the script never checks for it up front.
Why: On systems without jq, the script will fail mid-way with a non-obvious error, even though jq is a hard dependency for core functionality.
How to Fix: Add an early dependency check for jq (and optionally curl) near the top of the script, with a clear error message if it’s missing.
| PR_MERGED_COUNT=$(echo "$PR_LIST_RESPONSE" | jq '.total_count') | |
| # Color codes | |
| if ! command -v jq >/dev/null 2>&1; then | |
| echo "❌ Error: 'jq' is required but not installed. Please install jq to use this script." | |
| exit 1 | |
| fi | |
| PR_MERGED_COUNT=$(jq '.total_count' "$PR_LIST_RESPONSE_FILE") | |
| # Color codes |
| echo "==============================================================================" | ||
| echo -e "## ${GREEN}CHANGELOG${NOCOLOR}" | ||
| echo "$PR_LIST_RESPONSE" | jq -r '.items[] | (.title) + " (" + (.user.login) + ") [#" + (.number | tostring) + "]"' | sort | ||
| printf "\n" |
There was a problem hiding this comment.
Priority: 🟡 MEDIUM
Problem: The script assumes .items always exists and is an array, and pipes directly to jq -r '.items[] | ...', which will fail with a non-zero exit code if there are no items or if the structure is slightly different.
Why: An empty result set (no merged PRs in the period) is a valid scenario and should not cause the script to error; instead, it should print an empty changelog or a friendly message.
How to Fix: Guard the iteration with a check on PR_MERGED_COUNT or use jq options that handle empty arrays gracefully, and print a clear message when there are zero PRs.
| echo "==============================================================================" | |
| echo -e "## ${GREEN}CHANGELOG${NOCOLOR}" | |
| echo "$PR_LIST_RESPONSE" | jq -r '.items[] | (.title) + " (" + (.user.login) + ") [#" + (.number | tostring) + "]"' | sort | |
| printf "\n" | |
| echo "==============================================================================" | |
| echo -e "## ${GREEN}CHANGELOG${NOCOLOR}" | |
| if [ "$PR_MERGED_COUNT" -gt 0 ]; then | |
| jq -r '.items[] | (.title) + " (" + (.user.login) + ") [#" + (.number | tostring) + "]"' "$PR_LIST_RESPONSE_FILE" | sort | |
| else | |
| echo "No merged PRs found for the specified branch and date filter." | |
| fi | |
| printf "\n" |
| # Clean up temporary files | ||
| rm -f /tmp/curl_output.json /tmp/curl_error.log |
There was a problem hiding this comment.
Priority: 🟢 LOW
Problem: The cleanup step removes only /tmp/curl_output.json and /tmp/curl_error.log, which becomes incorrect once temp files are made unique (e.g., via mktemp), and it doesn’t clean up any additional temp files created during JSON cleaning.
Why: Leaving temp files behind clutters /tmp and can leak response data; attempting to remove hardcoded paths that no longer exist is harmless but misleading and suggests incomplete cleanup.
How to Fix: Track all temp file paths in variables and remove those specific files at the end of the script (and optionally via a trap to ensure cleanup on error).
| # Clean up temporary files | |
| rm -f /tmp/curl_output.json /tmp/curl_error.log | |
| # Clean up temporary files | |
| rm -f "$OUTPUT_FILE" "$ERROR_FILE" ${CLEANED_FILE:-} /tmp/invalid_json_debug.json 2>/dev/null || true |
| on: | ||
| pull_request: | ||
| branches: [master, main,staging, dev,develop] | ||
| types: [ready_for_review, reopened, review_requested, review_request_removed, opened, edited] |
There was a problem hiding this comment.
Priority: 🟡 MEDIUM
Problem: The branches list is missing spaces after some commas (main,staging, dev,develop), which is inconsistent and can reduce readability/maintainability of the workflow configuration.
Why: While GitHub Actions will still parse this correctly, inconsistent formatting makes the workflow harder to scan and increases the chance of subtle mistakes when editing (e.g., accidentally merging branch names).
How to Fix: Normalize the list formatting by adding spaces after all commas so each branch name is clearly separated and the style is consistent.
| on: | |
| pull_request: | |
| branches: [master, main,staging, dev,develop] | |
| types: [ready_for_review, reopened, review_requested, review_request_removed, opened, edited] | |
| on: | |
| pull_request: | |
| branches: [master, main, staging, dev, develop] | |
| types: [ready_for_review, reopened, review_requested, review_request_removed, opened, edited] |
| env: | ||
| BYPASS_LABEL: pr-size-exception | ||
| environment: ${{ contains(github.event.pull_request.labels.*.name, 'pr-size-exception') && 'cb-billing-reviewers' || '' }} | ||
| steps: | ||
| - uses: chargebee/cb-cicd-pipelines/.github/actions/pr-size-check@v4.20.3 |
There was a problem hiding this comment.
Priority: 🟡 MEDIUM
Problem: The environment is set to an empty string ('') when the pr-size-exception label is not present, which is not a valid environment name and can cause the job to fail or behave unexpectedly.
Why: GitHub Actions expects environment to be either omitted or set to a valid environment name; providing an empty string may result in runtime errors or undefined behavior, breaking the PR size check workflow for normal PRs.
How to Fix: Make the environment assignment conditional so that it is only set when the bypass label is present, and omitted otherwise (e.g., using a ternary that returns null instead of '', or by moving the environment to a separate job or using if on the job).
| env: | |
| BYPASS_LABEL: pr-size-exception | |
| environment: ${{ contains(github.event.pull_request.labels.*.name, 'pr-size-exception') && 'cb-billing-reviewers' || '' }} | |
| steps: | |
| - uses: chargebee/cb-cicd-pipelines/.github/actions/pr-size-check@v4.20.3 | |
| env: | |
| BYPASS_LABEL: pr-size-exception | |
| environment: ${{ contains(github.event.pull_request.labels.*.name, 'pr-size-exception') && 'cb-billing-reviewers' || null }} | |
| steps: | |
| - uses: chargebee/cb-cicd-pipelines/.github/actions/pr-size-check@v4.20.3 |
| excludePaths: | | ||
| .github/** | ||
| .cursor/** | ||
|
|
||
|
|
||
| - name: Ensure required check passes when bypassed |
There was a problem hiding this comment.
Priority: 🟢 LOW
Problem: There are two trailing blank lines inside the excludePaths block, which are unnecessary and can make the YAML less tidy or potentially confusing for future edits.
Why: While YAML will generally ignore these blank lines, keeping configuration files clean and minimal improves readability and reduces the chance of accidental indentation or formatting errors in future modifications.
How to Fix: Remove the extra blank lines at the end of the excludePaths list so that only the intended paths are present.
| excludePaths: | | |
| .github/** | |
| .cursor/** | |
| - name: Ensure required check passes when bypassed | |
| excludePaths: | | |
| .github/** | |
| .cursor/** | |
| - name: Ensure required check passes when bypassed |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
1 participant
This PR adds standardized PR template, changelog script, PR lint workflow, and PR size check workflow.