Quality Gates: PR Title standardization and LOC#120
Quality Gates: PR Title standardization and LOC#120cb-anomitromunshi wants to merge 6 commits intomainfrom
Conversation
✅ 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. |
![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 - Simple
View Breakdown
- Lines Changed: 239
- Files Changed: 5
- Complexity Added: 0
- Raw Score: 19.78
⚠️ Sensitive Data Detected
| File | Types | Count | ||||||
|---|---|---|---|---|---|---|---|---|
|
| Line | Type | Preview |
|---|---|---|
| 13 | Secret: Secret Keyword | [Secret Keyword] |
.github/workflows/pr-size-check.yml
| Line | Type | Preview |
|---|---|---|
| 67 | PII: Phone Number | 127322177288 |
High-level summary
This PR introduces standardized tooling and automation around pull requests and release management:
- Adds a PR template to enforce consistent PR descriptions, changelog entries, and documentation links.
- Introduces a script and documentation to generate changelogs from merged PRs via the GitHub API.
- Adds a reusable PR lint workflow for base branches
mainandmaster. - Adds a PR size check workflow for
devanddevelop/**branches, including a bypass mechanism with audit logging to S3.
Key functionalities and changes
-
PR Template
- Standardizes PR metadata: changelog, summary, areas of impact, test report URL, type of change, and documentation links.
- Encourages explicit marking of automation-related changes.
-
Changelog Generation
- New
generate-changelog.shscript to list merged PRs into a given branch over a date range. - Filters out “Parent branch sync” and bot-authored PRs.
- Includes robust error handling for GitHub API calls and JSON parsing.
- README added with usage instructions and examples.
- New
-
PR Lint Workflow
- Adds a common PR lint job that runs on PR events targeting
mainormaster. - Delegates checks to a shared workflow in
chargebee/cb-cicd-pipelines.
- Adds a common PR lint job that runs on PR events targeting
-
PR Size Check Workflow
- Enforces PR size thresholds on
devanddevelop/**branches. - Supports a
pr-size-exceptionlabel to bypass size enforcement. - Posts a comment when bypass approval is pending and requires CAB reviewers.
- When bypassed, records approval metadata to an S3 bucket via AWS OIDC.
- Enforces PR size thresholds on
File-level change summary
| File | Change summary |
|---|---|
.github/pull_request_template.md |
Adds a standardized PR template capturing changelog, summary, automation test details, areas of impact, type of change (bugfix/feature/enhancement/tests/docs/chore), and documentation links. |
.github/scripts/README.md |
Documents the new changelog generation script, including prerequisites (jq, GitHub credentials), environment setup, example commands, and argument behavior. |
.github/scripts/generate-changelog.sh |
New Bash script that queries the GitHub Search API for merged PRs into a specified branch within a date range, filters out sync/bot PRs, handles API errors and invalid JSON, prints a formatted changelog and a verification search URL, and cleans up temp files. |
.github/workflows/pr-lint.yml |
Adds a “Common PR Lint” GitHub Actions workflow that triggers on various PR events for key branches and runs a shared lint workflow from chargebee/cb-cicd-pipelines, limited to PRs targeting main or master. |
.github/workflows/pr-size-check.yml |
Adds a “PR Size Check” workflow for dev and develop/** branches: posts a comment when a pr-size-exception label is present, runs a shared size-check action with warning/error thresholds, supports bypass via label, configures AWS OIDC when bypassed, and records bypass approvals to S3 for auditing. |
| ## CHANGELOG | ||
| REPLACE_ME_WITH_CHANGELOG | ||
|
|
||
| ## SUMMARY | ||
| REPLACE_ME_WITH_SUMMARY_OF_THE_CHANGES |
There was a problem hiding this comment.
Priority: 🟢 LOW
Problem: The template starts directly with a level-2 heading (## CHANGELOG) instead of a single top-level heading, which can reduce readability and consistency with common PR template structures.
Why: Having a clear top-level title (e.g., "Pull Request Template" or "PR Details") improves readability, especially when rendered in GitHub’s UI, and makes it easier for contributors to understand the structure at a glance.
How to Fix: Add a level-1 heading at the top of the file and keep the existing sections as subsections.
| ## CHANGELOG | |
| REPLACE_ME_WITH_CHANGELOG | |
| ## SUMMARY | |
| REPLACE_ME_WITH_SUMMARY_OF_THE_CHANGES | |
| # Pull Request Details | |
| ## CHANGELOG | |
| REPLACE_ME_WITH_CHANGELOG | |
| ## SUMMARY | |
| REPLACE_ME_WITH_SUMMARY_OF_THE_CHANGES |
| ## FUNCTIONAL AUTOMATION CHANGES PR | ||
| - [ ] Yes | ||
| - If Yes, PR : | ||
| - [ ] No | ||
| - If No, Reason: |
There was a problem hiding this comment.
Priority: 🟢 LOW
Problem: The "FUNCTIONAL AUTOMATION CHANGES PR" section uses free-text fields ("If Yes, PR :" / "If No, Reason:") which may lead to inconsistent input and missing links or reasons.
Why: Inconsistent formatting makes it harder to parse PRs (both for humans and any future automation) and increases the chance that authors forget to provide the required PR link or reason.
How to Fix: Replace the free-text prompts with clearly labeled placeholders on the same line, making it obvious what needs to be filled and encouraging consistent formatting.
| ## FUNCTIONAL AUTOMATION CHANGES PR | |
| - [ ] Yes | |
| - If Yes, PR : | |
| - [ ] No | |
| - If No, Reason: | |
| ## FUNCTIONAL AUTOMATION CHANGES PR | |
| - [ ] Yes — Related automation PR: REPLACE_ME_WITH_AUTOMATION_PR_LINK | |
| - [ ] No — Reason: REPLACE_ME_WITH_REASON_FOR_NO_AUTOMATION_CHANGES |
| ## AUTOMATION TEST REPORT URL | ||
| REPLACE_ME_WITH_TEST_REPORT_URL |
There was a problem hiding this comment.
Priority: 🟢 LOW
Problem: The "AUTOMATION TEST REPORT URL" section uses a generic placeholder without clarifying that "N/A" is acceptable when not applicable.
Why: Without explicit guidance, contributors may leave this blank or provide inconsistent values, making it harder to quickly see whether tests were run or are not applicable.
How to Fix: Update the placeholder to explicitly allow "N/A" and encourage a consistent format.
| ## AUTOMATION TEST REPORT URL | |
| REPLACE_ME_WITH_TEST_REPORT_URL | |
| ## AUTOMATION TEST REPORT URL | |
| REPLACE_ME_WITH_TEST_REPORT_URL_OR_NA |
| ## AREAS OF IMPACT | ||
| REPLACE_ME_WITH_AREAS_OF_IMPACT_OR_NA |
There was a problem hiding this comment.
Priority: 🟢 LOW
Problem: The "AREAS OF IMPACT" section uses a free-text placeholder only, which may lead to vague or missing impact descriptions.
Why: Clear, structured impact information helps reviewers quickly understand risk and affected components; unstructured text can be inconsistent and less useful.
How to Fix: Provide examples or a suggested format (e.g., bullet list or component names) to guide authors toward more actionable impact descriptions.
| ## AREAS OF IMPACT | |
| REPLACE_ME_WITH_AREAS_OF_IMPACT_OR_NA | |
| ## AREAS OF IMPACT | |
| REPLACE_ME_WITH_AREAS_OF_IMPACT_OR_NA | |
| <!-- Example: Billing API, Invoicing UI, Webhooks, Reporting, Docs --> |
| ## TYPE OF CHANGE | ||
| - [ ] 🐞 Bugfix | ||
| - [ ] 🌟 Feature | ||
| - [ ] ✨ Enhancement | ||
| - [ ] 🧪 Unit Test Cases | ||
| - [ ] 📔 Documentation | ||
| - [ ] ⚙️ Chore - Build Related / Configuration / Others |
There was a problem hiding this comment.
Priority: 🟢 LOW
Problem: The "TYPE OF CHANGE" section uses emojis, which may not render consistently in all environments and can be noisy for any future automated parsing.
Why: While emojis are visually helpful, they can hinder machine readability and may not be accessible in all contexts (e.g., screen readers or text-only views).
How to Fix: Keep the labels but move emojis to comments or remove them, ensuring the primary information is plain text.
| ## TYPE OF CHANGE | |
| - [ ] 🐞 Bugfix | |
| - [ ] 🌟 Feature | |
| - [ ] ✨ Enhancement | |
| - [ ] 🧪 Unit Test Cases | |
| - [ ] 📔 Documentation | |
| - [ ] ⚙️ Chore - Build Related / Configuration / Others | |
| ## TYPE OF CHANGE | |
| - [ ] Bugfix | |
| - [ ] Feature | |
| - [ ] Enhancement | |
| - [ ] Unit Test Cases | |
| - [ ] Documentation | |
| - [ ] Chore - Build Related / Configuration / Others |
| ## 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 has no trailing newline (\n), as indicated by \ No newline at end of file.
Why: Many tools and linters expect a trailing newline at the end of text files; missing it can cause unnecessary diffs and minor tooling issues.
How to Fix: Add a newline at the end of the file.
| ## 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-flutter" |
There was a problem hiding this comment.
Priority: 🟠 HIGH
Problem: The DATE_FILTER default uses date -v-30d (BSD/macOS) with a fallback to date -d '30 days ago' (GNU/Linux), but the || fallback is inside the command substitution, so on systems where the first date variant exists but fails for another reason (e.g., locale/format issues), the whole script will still exit due to set -e and never reach the fallback.
Why: This makes the script brittle and potentially non-portable across environments; a transient failure in the first date invocation will cause the entire script to exit instead of gracefully trying the alternative, which is especially problematic for CI or shared tooling.
How to Fix: Move the fallback logic outside the command substitution and explicitly try BSD-style date first, then GNU-style date, and finally exit with a clear error if both fail, so set -e does not prevent the fallback from running.
| # 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-flutter" | |
| # Optional: Date filter (defaults to last 30 days if not provided) | |
| if [[ -z "$2" ]]; then | |
| if DATE_VALUE=$(date -u -v-30d +%Y-%m-%d 2>/dev/null); then | |
| DATE_FILTER="merged:>=$DATE_VALUE" | |
| elif DATE_VALUE=$(date -u -d '30 days ago' +%Y-%m-%d 2>/dev/null); then | |
| DATE_FILTER="merged:>=$DATE_VALUE" | |
| else | |
| echo "❌ Error: Unable to compute date 30 days ago with 'date' command" | |
| exit 1 | |
| fi | |
| else | |
| DATE_FILTER="$2" | |
| fi | |
| # Repo is set per-repo when this file is pushed (placeholder replaced by upload script) | |
| REPO="chargebee/chargebee-flutter" |
| echo "🌐 API call status: $HTTP_STATUS" | ||
|
|
||
| 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 [ "$HTTP_STATUS" -ne 200 ]; then | ||
| echo "❌ Error: API returned HTTP status $HTTP_STATUS" | ||
| echo "Response: $(cat /tmp/curl_output.json)" | ||
| exit 1 | ||
| fi | ||
|
|
||
| PR_LIST_RESPONSE=$(cat /tmp/curl_output.json) |
There was a problem hiding this comment.
Priority: 🟡 MEDIUM
Problem: The script logs the full HTTP status line (🌐 API call status: $HTTP_STATUS), which includes the numeric status code but not the request URL or query; however, the subsequent error message on non-200 responses prints the entire JSON response inline, which can be very large and noisy for users.
Why: Dumping the full JSON response directly to stdout on error can make it hard to see the actual error cause, especially if the response contains many items; it also makes logs harder to scan and may expose more data than necessary in CI logs.
How to Fix: Print a concise error summary (status code and message) and write the full JSON response to a temporary debug file, similar to how invalid JSON is handled later, so users can inspect details only when needed.
| echo "🌐 API call status: $HTTP_STATUS" | |
| 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 [ "$HTTP_STATUS" -ne 200 ]; then | |
| echo "❌ Error: API returned HTTP status $HTTP_STATUS" | |
| echo "Response: $(cat /tmp/curl_output.json)" | |
| exit 1 | |
| fi | |
| PR_LIST_RESPONSE=$(cat /tmp/curl_output.json) | |
| echo "🌐 API call status: $HTTP_STATUS" | |
| 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 [ "$HTTP_STATUS" -ne 200 ]; then | |
| cp /tmp/curl_output.json /tmp/changelog_api_error.json 2>/dev/null || true | |
| echo "❌ Error: API returned HTTP status $HTTP_STATUS" | |
| echo "💡 Full response saved to /tmp/changelog_api_error.json for inspection" | |
| exit 1 | |
| fi | |
| PR_LIST_RESPONSE=$(cat /tmp/curl_output.json) |
| echo "==============================================================================" | ||
| echo -e "Found ${GREEN}$PR_MERGED_COUNT${NOCOLOR} PR(s) merged into $SOURCE_BRANCH (filter: $DATE_FILTER)" | ||
| 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 that .items exists and is an array when piping to jq -r '.items[] | ...', but it does not guard against the case where .items is missing or not an array (e.g., unexpected API response shape), which would cause jq to exit non-zero and terminate the script due to set -e.
Why: A schema change or partial API failure could result in a different JSON structure, causing the script to crash instead of failing gracefully with a clear message; this reduces robustness of the tooling.
How to Fix: Add a defensive check that .items is an array before iterating, and if not, print a clear error and exit, or handle the empty/non-array case gracefully.
| echo "==============================================================================" | |
| echo -e "Found ${GREEN}$PR_MERGED_COUNT${NOCOLOR} PR(s) merged into $SOURCE_BRANCH (filter: $DATE_FILTER)" | |
| echo "==============================================================================" | |
| echo -e "## ${GREEN}CHANGELOG${NOCOLOR}" | |
| echo "$PR_LIST_RESPONSE" | jq -r '.items[] | (.title) + " (" + (.user.login) + ") [#" + (.number | tostring) + "]"' | sort | |
| printf "\n" | |
| echo "==============================================================================" | |
| echo -e "Found ${GREEN}$PR_MERGED_COUNT${NOCOLOR} PR(s) merged into $SOURCE_BRANCH (filter: $DATE_FILTER)" | |
| echo "==============================================================================" | |
| echo -e "## ${GREEN}CHANGELOG${NOCOLOR}" | |
| if echo "$PR_LIST_RESPONSE" | jq -e '.items | type == "array"' >/dev/null 2>&1; then | |
| echo "$PR_LIST_RESPONSE" | jq -r '.items[] | (.title) + " (" + (.user.login) + ") [#" + (.number | tostring) + "]"' | sort | |
| else | |
| echo "⚠️ Unexpected API response format: '.items' is missing or not an array" | |
| fi | |
| printf "\n" |
| echo "==============================================================================" | ||
| echo -e "${GREEN}GitHub Search URL (to verify, if required)${NOCOLOR}" | ||
| BRANCH_ENCODED=$(echo "$SOURCE_BRANCH" | sed 's/ /%20/g') | ||
| echo "https://github.com/$REPO/pulls?q=NOT+%22Parent+branch+sync%22+in%3Atitle+is%3Apr+is%3Amerged+base%3A$BRANCH_ENCODED+merged%3A$DATE_FILTER+-author%3Aapp%2Fdistributed-gitflow-app" | ||
| echo "==============================================================================" |
There was a problem hiding this comment.
Priority: 🟢 LOW
Problem: The branch name is URL-encoded only for spaces (sed 's/ /%20/g'), but other characters that are valid in branch names (e.g., #, +, /) are not encoded, which can produce an invalid or misleading GitHub search URL.
Why: An incorrectly encoded URL may not reproduce the same search results in the browser, reducing the usefulness of the “GitHub Search URL” verification link, especially for branches with special characters.
How to Fix: Use a more robust URL-encoding approach for the branch name, for example by leveraging python -c or perl to percent-encode all non-safe characters.
| echo "==============================================================================" | |
| echo -e "${GREEN}GitHub Search URL (to verify, if required)${NOCOLOR}" | |
| BRANCH_ENCODED=$(echo "$SOURCE_BRANCH" | sed 's/ /%20/g') | |
| echo "https://github.com/$REPO/pulls?q=NOT+%22Parent+branch+sync%22+in%3Atitle+is%3Apr+is%3Amerged+base%3A$BRANCH_ENCODED+merged%3A$DATE_FILTER+-author%3Aapp%2Fdistributed-gitflow-app" | |
| echo "==============================================================================" | |
| echo "==============================================================================" | |
| echo -e "${GREEN}GitHub Search URL (to verify, if required)${NOCOLOR}" | |
| BRANCH_ENCODED=$(python - <<'PYCODE' | |
| import sys, urllib.parse | |
| print(urllib.parse.quote(sys.argv[1], safe='')) | |
| PYCODE | |
| "$SOURCE_BRANCH") | |
| echo "https://github.com/$REPO/pulls?q=NOT+%22Parent+branch+sync%22+in%3Atitle+is%3Apr+is%3Amerged+base%3A$BRANCH_ENCODED+merged%3A$DATE_FILTER+-author%3Aapp%2Fdistributed-gitflow-app" | |
| echo "==============================================================================" |
| on: | ||
| pull_request: | ||
| branches: [master, main,staging, dev,develop] | ||
| types: [ready_for_review, reopened, review_requested, review_request_removed, opened, edited] | ||
|
|
||
| jobs: | ||
| pr-lint: | ||
| name: Common PR Lint Checks | ||
| if: github.base_ref == 'main' || github.base_ref == 'master' | ||
| uses: chargebee/cb-cicd-pipelines/.github/workflows/pr-lint.yml@main |
There was a problem hiding this comment.
Priority: 🟡 MEDIUM
Problem: The branches list in the pull_request trigger includes staging, dev, and develop, but the job is hard-filtered to only run when github.base_ref is main or master, making those extra branches misleading and unnecessary.
Why: Having branches in the trigger that can never satisfy the job condition can confuse maintainers about which branches are actually linted and may lead to incorrect assumptions about coverage; it also adds noise to the workflow configuration.
How to Fix: Either remove the unused branches from the trigger to reflect the actual behavior, or, if linting is desired on those branches, update the if condition to include them so the job runs as expected.
| on: | |
| pull_request: | |
| branches: [master, main,staging, dev,develop] | |
| types: [ready_for_review, reopened, review_requested, review_request_removed, opened, edited] | |
| jobs: | |
| pr-lint: | |
| name: Common PR Lint Checks | |
| if: github.base_ref == 'main' || github.base_ref == 'master' | |
| uses: chargebee/cb-cicd-pipelines/.github/workflows/pr-lint.yml@main | |
| on: | |
| pull_request: | |
| branches: [master, main] | |
| types: [ready_for_review, reopened, review_requested, review_request_removed, opened, edited] | |
| jobs: | |
| pr-lint: | |
| name: Common PR Lint Checks | |
| if: github.base_ref == 'main' || github.base_ref == 'master' | |
| uses: chargebee/cb-cicd-pipelines/.github/workflows/pr-lint.yml@main |
| pull_request: | ||
| branches: [master, main,staging, dev,develop] | ||
| types: [ready_for_review, reopened, review_requested, review_request_removed, opened, edited] | ||
|
|
||
| jobs: |
There was a problem hiding this comment.
Priority: 🟢 LOW
Problem: The types list for the pull_request trigger omits the synchronize event, so PR lint checks will not rerun when commits are pushed to an existing PR.
Why: Without handling synchronize, changes pushed after initial PR creation (e.g., addressing review comments) will not trigger the lint workflow, potentially allowing regressions or missing required checks on updated code.
How to Fix: Add synchronize to the types array so that the workflow runs whenever new commits are pushed to an open pull request.
| pull_request: | |
| branches: [master, main,staging, dev,develop] | |
| types: [ready_for_review, reopened, review_requested, review_request_removed, opened, edited] | |
| jobs: | |
| pull_request: | |
| branches: [master, main,staging, dev,develop] | |
| types: [ready_for_review, reopened, review_requested, review_request_removed, opened, edited, synchronize] | |
| jobs: |
| - uses: actions/github-script@v7 | ||
| with: | ||
| github-token: ${{ secrets.GITHUB_TOKEN }} | ||
| script: | | ||
| const owner = context.repo.owner; | ||
| const repo = context.repo.repo; | ||
| const issue_number = context.payload.pull_request.number; | ||
|
|
||
| const marker = '<!-- pr-size-bypass-pending -->'; | ||
| const pending = `${marker} | ||
| 🛑 The \`pr-size-exception\` label is present. This workflow is **waiting for approvals** from the **[cb-Billing-CAB-reviewers](https://github.com/orgs/chargebee/teams/cb-billing-cab-approvers)**.`; | ||
|
|
||
| // create a new comment when the workflow runs | ||
| await github.rest.issues.createComment({ owner, repo, issue_number, body: pending }); |
There was a problem hiding this comment.
Priority: 🟠 HIGH
Problem: The pre-approval-comment job runs on every qualifying event with the pr-size-exception label and always posts a new comment, which will spam the PR with duplicate “pending bypass approval” comments.
Why: On every synchronize, edited, labeled, unlabeled, or reopened event while the label is present, this job will add another identical comment, cluttering the PR discussion and making it harder to track real reviewer feedback.
How to Fix: Before creating a new comment, query existing comments for the marker (<!-- pr-size-bypass-pending -->) and only create a comment if one does not already exist.
| - uses: actions/github-script@v7 | |
| with: | |
| github-token: ${{ secrets.GITHUB_TOKEN }} | |
| script: | | |
| const owner = context.repo.owner; | |
| const repo = context.repo.repo; | |
| const issue_number = context.payload.pull_request.number; | |
| const marker = '<!-- pr-size-bypass-pending -->'; | |
| const pending = `${marker} | |
| 🛑 The \`pr-size-exception\` label is present. This workflow is **waiting for approvals** from the **[cb-Billing-CAB-reviewers](https://github.com/orgs/chargebee/teams/cb-billing-cab-approvers)**.`; | |
| // create a new comment when the workflow runs | |
| await github.rest.issues.createComment({ owner, repo, issue_number, body: pending }); | |
| - uses: actions/github-script@v7 | |
| with: | |
| github-token: ${{ secrets.GITHUB_TOKEN }} | |
| script: | | |
| const owner = context.repo.owner; | |
| const repo = context.repo.repo; | |
| const issue_number = context.payload.pull_request.number; | |
| const marker = '<!-- pr-size-bypass-pending -->'; | |
| const pending = `${marker} | |
| 🛑 The \`pr-size-exception\` label is present. This workflow is **waiting for approvals** from the **[cb-Billing-CAB-reviewers](https://github.com/orgs/chargebee/teams/cb-billing-cab-approvers)**.`; | |
| // Check if a pending marker comment already exists | |
| const { data: comments } = await github.rest.issues.listComments({ | |
| owner, | |
| repo, | |
| issue_number, | |
| per_page: 100, | |
| }); | |
| const alreadyPending = comments.some(comment => comment.body && comment.body.includes(marker)); | |
| if (!alreadyPending) { | |
| await github.rest.issues.createComment({ owner, repo, issue_number, body: pending }); | |
| } |
| 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 may 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 can lead to configuration errors or undefined behavior, especially if environments are used for protection rules.
How to Fix: Use a conditional expression that omits the environment when the label is not present by moving the condition into the job if or by splitting into two jobs, or use a separate job that only runs with the environment when the label is present. Within a single job, the safest approach is to remove environment and instead gate the OIDC/S3 steps on the label (which you already do).
| 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 | |
| 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 consecutive blank lines in the excludePaths block, which is minor formatting noise and can be confusing when scanning the workflow.
Why: Extra blank lines inside YAML blocks can make the configuration look untidy and, in some editors or linters, may be flagged as style issues, slightly reducing readability.
How to Fix: Remove the redundant blank lines so that the excludePaths list is compact and consistent.
| excludePaths: | | |
| .github/** | |
| .cursor/** | |
| - name: Ensure required check passes when bypassed | |
| excludePaths: | | |
| .github/** | |
| .cursor/** | |
| - name: Ensure required check passes when bypassed |
There was a problem hiding this comment.
PR Complexity Score: 1.2 - Trivial
View Breakdown
- Lines Changed: 81
- Files Changed: 1
- Complexity Added: 0
- Raw Score: 4.62
⚠️ Sensitive Data Detected
| File | Types | Count | ||||||
|---|---|---|---|---|---|---|---|---|
|
| Line | Type | Preview |
|---|---|---|
| 13 | Secret: Secret Keyword | [Secret Keyword] |
.github/workflows/pr-size-check.yml
| Line | Type | Preview |
|---|---|---|
| 66 | PII: Phone Number | 127322177288 |
- Introduces an automated PR size checking workflow for PRs targeting
main. - Enforces size thresholds (warning at 200 lines, error at 250 lines) using a shared
pr-size-checkaction. - Excludes certain paths (e.g.,
.github/**,.cursor/**) from size calculations. - Adds a bypass mechanism via the
pr-size-exceptionlabel, including:- A pre-approval comment notifying that CAB approval is required.
- An environment-gated flow (
cb-billing-reviewers) for labeled PRs. - Recording of bypass approvals to an S3 bucket via OIDC-authenticated AWS credentials.
High-level purpose and impact:
- Purpose: Automatically monitor and control PR size, while allowing controlled exceptions that are auditable.
- Key areas affected: GitHub Actions CI configuration, PR review process, and compliance/audit trail via S3 logging.
File-level change summary
| File | Change Summary |
|---|---|
.github/workflows/pr-size-check.yml |
New GitHub Actions workflow that: (1) posts a comment when pr-size-exception is present and approvals are pending; (2) runs a PR size check for PRs into main using chargebee/cb-cicd-pipelines with defined size thresholds and excluded paths; (3) supports a bypass path when the pr-size-exception label is applied, ensuring the check passes, configuring AWS OIDC credentials, and recording bypass approval metadata (repo, date, PR link, workflow ID) to an S3 bucket. |
| - name: Record bypass approval to S3 | ||
| if: ${{ contains(github.event.pull_request.labels.*.name, env.BYPASS_LABEL) }} | ||
| run: | | ||
| REPO="${{ github.repository }}" | ||
| PR_LINK="https://github.com/${{ github.repository }}/pull/${{ github.event.pull_request.number }}" | ||
| DATE="$(date -u +%Y-%m-%dT%H:%M:%SZ)" | ||
| WF_ID="${{ github.run_id }}" | ||
| S3_KEY="${REPO}/datas/${WF_ID}.json" | ||
| printf '{"repo":"%s","date":"%s","pr_link":"%s","wf_id":"%s"}\n' "$REPO" "$DATE" "$PR_LINK" "$WF_ID" | \ | ||
| aws s3 cp - "s3://prsizebypassdata/${S3_KEY}" --content-type application/json | ||
| echo "Recorded to s3://prsizebypassdata/${S3_KEY}" |
There was a problem hiding this comment.
Priority: 🟡 MEDIUM
Problem: The S3 key and bucket name used to record bypass approvals are hard-coded (prsizebypassdata and datas), which makes this workflow less portable and harder to adjust across environments or repositories.
Why: Hard-coded infrastructure identifiers couple the workflow tightly to a specific AWS setup; any change to bucket naming, prefixes, or multi-environment support will require code changes instead of configuration changes, increasing maintenance overhead and risk of misconfiguration.
How to Fix: Move the bucket name and (optionally) the prefix into workflow-level env variables or repository/organization secrets, and reference those variables in the script so they can be updated without modifying the workflow logic.
| - name: Record bypass approval to S3 | |
| if: ${{ contains(github.event.pull_request.labels.*.name, env.BYPASS_LABEL) }} | |
| run: | | |
| REPO="${{ github.repository }}" | |
| PR_LINK="https://github.com/${{ github.repository }}/pull/${{ github.event.pull_request.number }}" | |
| DATE="$(date -u +%Y-%m-%dT%H:%M:%SZ)" | |
| WF_ID="${{ github.run_id }}" | |
| S3_KEY="${REPO}/datas/${WF_ID}.json" | |
| printf '{"repo":"%s","date":"%s","pr_link":"%s","wf_id":"%s"}\n' "$REPO" "$DATE" "$PR_LINK" "$WF_ID" | \ | |
| aws s3 cp - "s3://prsizebypassdata/${S3_KEY}" --content-type application/json | |
| echo "Recorded to s3://prsizebypassdata/${S3_KEY}" | |
| - name: Record bypass approval to S3 | |
| if: ${{ contains(github.event.pull_request.labels.*.name, env.BYPASS_LABEL) }} | |
| env: | |
| PR_SIZE_BYPASS_BUCKET: prsizebypassdata | |
| PR_SIZE_BYPASS_PREFIX: datas | |
| run: | | |
| REPO="${{ github.repository }}" | |
| PR_LINK="https://github.com/${{ github.repository }}/pull/${{ github.event.pull_request.number }}" | |
| DATE="$(date -u +%Y-%m-%dT%H:%M:%SZ)" | |
| WF_ID="${{ github.run_id }}" | |
| S3_KEY="${REPO}/${PR_SIZE_BYPASS_PREFIX}/${WF_ID}.json" | |
| printf '{"repo":"%s","date":"%s","pr_link":"%s","wf_id":"%s"}\n' "$REPO" "$DATE" "$PR_LINK" "$WF_ID" | \ | |
| aws s3 cp - "s3://${PR_SIZE_BYPASS_BUCKET}/${S3_KEY}" --content-type application/json | |
| echo "Recorded to s3://${PR_SIZE_BYPASS_BUCKET}/${S3_KEY}" |
1 participant
This PR adds standardized PR template, changelog script, PR lint workflow, and PR size check workflow.