Skip to content

fix(go-pr-analysis): warn when custom_checks is enabled but empty - #626

Merged
bedatty merged 1 commit into
developfrom
fix/custom-checks-empty-and-fallback-comment
Jul 27, 2026
Merged

fix(go-pr-analysis): warn when custom_checks is enabled but empty#626
bedatty merged 1 commit into
developfrom
fix/custom-checks-empty-and-fallback-comment

Conversation

@bedatty

@bedatty bedatty commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Description

Two follow-ups from the CodeRabbit review on #625 (the developmain promotion carrying #623 and #624).

1. custom_checks enabled but empty no longer skips silently

The job was gated on inputs.custom_checks != '', so a caller that set enable_custom_checks: true and forgot the list got a silently absent job — a gate reporting coverage it never provided. Dropped that clause from the if: and added an early guard in the run step:

if [ -z "${CUSTOM_CHECKS//[[:space:]]/}" ]; then
  echo "::warning::enable_custom_checks is true but custom_checks is empty — no checks ran"
  exit 0
fi

Whitespace-stripped, so a list containing only newlines warns too. Costs a runner spin-up on a misconfigured repo; that seemed clearly better than a gate that quietly claims to have run.

2. Corrected a misleading comment on the integration_test_command fallback

The comment claimed empty "falls through to go-pr-analysis's own default". It does not — the umbrella substitutes the same literal. The code is correct and load-bearing; only the comment was wrong.

The reviewer's proposed fix was to drop the fallback and pass the input through as-is. That would break callers: a reusable-workflow default applies only when the input is omitted, and a with: mapping cannot conditionally omit a key. An empty string therefore arrives at go-pr-analysis as '', and its integration-tests job does run: ${{ inputs.integration_test_command }} — an empty run: is a workflow validation error. Every umbrella caller with enable_integration_tests: true and no explicit command would fail.

So the literal has to be repeated. The comment now says that, and says why, and flags it as a keep-in-sync duplicate.

Type of Change

  • fix: Bug fix in a workflow (incorrect behavior, broken step, wrong condition)

Breaking Changes

None. Change 1 only affects a configuration that was previously a silent no-op — it now emits a warning and still exits 0, so no currently-passing PR starts failing. Change 2 is comment-only.

Testing

  • YAML syntax validated locally
  • Triggered a real workflow run on a caller repository using @this-branch or the beta tag
  • Verified all existing inputs still work with default values
  • Confirmed no secrets or tokens are printed in logs
  • Checked that unrelated workflows are not affected

Parsed both workflows and asserted the custom-checks if: no longer references custom_checks, that the guard is the first thing in the run step, and that the forwarded integration_test_command expression is unchanged. Ran the guard's bash condition against three inputs — empty, newlines-only, and a real target list — confirming the first two warn and exit 0 while the third proceeds.

The happy path was already exercised end to end on LerianStudio/caradhras PR #34, where Custom Checks (caradhras) ran check-migrations, check-goroutines and check-e2e-vet and agreed with the bespoke jobs kept as a control on the same commit.

Related Issues

Follow-up to #623 / #624, reviewed on #625.

@bedatty
bedatty requested a review from a team as a code owner July 27, 2026 21:45
@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown

Review Change Stack

Summary by CodeRabbit

  • Bug Fixes

    • Custom checks now run when enabled and Go changes are detected, even when no specific check targets are configured.
    • Empty or whitespace-only custom check configurations are handled safely with a warning instead of causing failures.
  • Documentation

    • Clarified integration test command configuration to prevent validation issues in reusable workflows.

Walkthrough

Changes

Custom checks execution

Layer / File(s) Summary
Custom checks gating and empty-input handling
.github/workflows/go-pr-analysis.yml
The custom-checks job runs when enabled and Go changes are detected, while empty or whitespace-only check lists produce a warning and successful exit.

Integration test command documentation

Layer / File(s) Summary
Integration test default clarification
.github/workflows/go-pr-validation.yml
Comments explain why the make test-integration default is repeated inline for the downstream workflow.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Suggested labels: workflow, golang, validate

Suggested reviewers: gandalf-at-lerian, fredcamaral

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title matches the main behavior change: warning when custom_checks is enabled but empty.
Description check ✅ Passed The description covers the affected workflows, behavior changes, type of change, breaking changes, testing, and related issues.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/custom-checks-empty-and-fallback-comment

Comment @coderabbitai help to get the list of available commands.

@lerian-studio lerian-studio added size/XS PR changes < 50 lines workflow Changes to one or more reusable workflow files golang Changes to Go-related workflows validate Changes to PR validation composite actions (src/validate/) labels Jul 27, 2026
@lerian-studio

Copy link
Copy Markdown
Contributor

🛡️ CodeQL Analysis Results

Languages analyzed: actions

✅ No security issues found.


🔍 View full scan logs | 🛡️ Security tab

@lerian-studio

Copy link
Copy Markdown
Contributor

🔍 Lint Analysis

Check Files Scanned Status
YAML Lint 2 file(s) ✅ success
Action Lint 2 file(s) ✅ success
Pinned Actions 2 file(s) ✅ success
Markdown Link Check no changes ⏭️ skipped
Spelling Check 2 file(s) ✅ success
Shell Check 2 file(s) ✅ success
README Check 2 file(s) ✅ success
Composite Schema no changes ⏭️ skipped
Deployment Matrix no changes ⏭️ skipped

🔍 View full scan logs

@lerian-studio

Copy link
Copy Markdown
Contributor

🔍 PR Validation Summary

✅ PR Mergeable — no blocking failures

Check Status Blocking
Source Branch ✅ success yes
PR Title ✅ success yes
PR Description ✅ success yes
PR Size ✅ success no
Auto Labels ✅ success no
PR Metadata ✅ success no

🔍 View workflow run

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warning

CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.

Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.

👉 Steps to fix this

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.github/workflows/go-pr-validation.yml:
- Around line 279-284: Complete the truncated explanatory comment above the
reusable workflow configuration by finishing the final sentence with a clear
reference to the downstream workflow, while preserving the existing explanation
and synchronization note.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: ASSERTIVE

Plan: Pro

Run ID: b6c30d97-3c4b-4b94-b76c-b5441ee6fe1a

📥 Commits

Reviewing files that changed from the base of the PR and between 198eaac and e1bd3dd.

📒 Files selected for processing (2)
  • .github/workflows/go-pr-analysis.yml
  • .github/workflows/go-pr-validation.yml

Comment thread .github/workflows/go-pr-validation.yml
@bedatty
bedatty merged commit 9da1f0a into develop Jul 27, 2026
19 checks passed
@github-actions
github-actions Bot deleted the fix/custom-checks-empty-and-fallback-comment branch July 27, 2026 21:49
@coderabbitai coderabbitai Bot mentioned this pull request Jul 27, 2026
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

golang Changes to Go-related workflows size/XS PR changes < 50 lines validate Changes to PR validation composite actions (src/validate/) workflow Changes to one or more reusable workflow files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants