Extract secret validation to lib.sh.#22
Conversation
|
Warning Review limit reached
More reviews will be available in 48 minutes and 51 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughInline secret and language-code validation checks in ChangesCentralized Secret Validation
Pinned Tool Versions for Linting
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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, full review. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
# Get PR details and diff
gh pr view 22 --repo cppalliance/boost-docs-translation --json title,body,files,additions,deletions
echo "===== DIFF ====="
gh pr diff 22 --repo cppalliance/boost-docs-translationLength of output: 8793 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Get the PR branch files
gh api repos/cppalliance/boost-docs-translation/contents/.github/workflows/assets/lib.sh \
--jq '.content' | base64 -d
echo "===== add-submodules.yml ====="
gh api repos/cppalliance/boost-docs-translation/contents/.github/workflows/add-submodules.yml \
-H "Accept: application/vnd.github.raw" 2>/dev/null || \
gh api repos/cppalliance/boost-docs-translation/git/trees/HEAD --jq '.tree[] | select(.path | contains("add-submodules"))' 2>/dev/null || true
echo "===== start-translation.yml ====="
gh api repos/cppalliance/boost-docs-translation/contents/.github/workflows/start-translation.yml \
-H "Accept: application/vnd.github.raw" 2>/dev/null || trueLength of output: 30736
Review: Extract secret validation to lib.shOverall: The centralization is well-structured. The three new helpers ( 🔴 Medium — Fail-fast regression in
|
|
Add a few bats cases in |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/test_lib.bats (2)
163-187: ⚡ Quick winConsider adding a test to verify the optional
weblateparameter gates correctly.The current tests verify that
validate_secretsrequiresGITHUB_TOKENandLANG_CODES, but there's no test confirming thatvalidate_secrets(without theweblateargument) succeeds whenWEBLATE_URLandWEBLATE_TOKENare unset. This would prove the optional parameter correctly gates Weblate-specific checks.🧪 Suggested additional test case
`@test` "validate_secrets: succeeds without Weblate secrets when weblate mode not requested" { load_env export GITHUB_TOKEN="test-token" export LANG_CODES="en" unset WEBLATE_URL unset WEBLATE_TOKEN run validate_secrets [ "$status" -eq 0 ] }🤖 Prompt for 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. In `@tests/test_lib.bats` around lines 163 - 187, The validate_secrets function appears to have optional Weblate-related checks that are gated by a parameter, but there is no test verifying that validate_secrets succeeds when called without the weblate argument and Weblate environment variables are unset. Add a new test case after the existing validate_secrets tests that calls validate_secrets with required variables GITHUB_TOKEN and LANG_CODES set, but with WEBLATE_URL and WEBLATE_TOKEN explicitly unset, and verify the function returns a successful exit status of 0. This confirms that Weblate-specific validation is properly gated by the optional parameter.
189-208: ⚡ Quick winConsider adding a test for missing
WEBLATE_TOKENin weblate mode.The test suite verifies that
validate_secrets weblatefails whenWEBLATE_URLis unset, but there's no corresponding test for whenWEBLATE_TOKENis unset. Adding this test would provide symmetric coverage of both Weblate-specific requirements.🧪 Suggested additional test case
`@test` "validate_secrets weblate: fails when WEBLATE_TOKEN is unset" { load_env export GITHUB_TOKEN="test-token" export LANG_CODES="en" export WEBLATE_URL="https://weblate.example.org" unset WEBLATE_TOKEN run validate_secrets weblate [ "$status" -eq 1 ] [[ "$output" == *"WEBLATE_TOKEN secret is not set"* ]] }🤖 Prompt for 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. In `@tests/test_lib.bats` around lines 189 - 208, Add a new test case after the "validate_secrets weblate: fails when WEBLATE_URL is unset" test that follows the same pattern but tests for missing WEBLATE_TOKEN instead. The test should load environment variables, set GITHUB_TOKEN, LANG_CODES, and WEBLATE_URL, explicitly unset WEBLATE_TOKEN, run validate_secrets weblate, assert the status is 1, and verify the output contains the message indicating WEBLATE_TOKEN secret is not set. This provides symmetric test coverage for both required Weblate configuration parameters.
🤖 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/start-translation.yml:
- Around line 109-110: The comment on line 109 incorrectly attributes the
diagnostic code SC2153 to actionlint when it actually originates from
ShellCheck. Update the comment text to replace "actionlint SC2153" with
"ShellCheck SC2153" to correctly identify the source of the diagnostic warning
about potentially misspelled or unset variables.
In `@scripts/lint.sh`:
- Around line 160-162: The _shellcheck_dir variable is incorrectly set using
dirname on SHELLCHECK_BIN, which points to the cache directory instead of the
actual directory containing the shellcheck binary. Since the shellcheck binary
is extracted to shellcheck-v0.11.0/shellcheck, modify the _shellcheck_dir
assignment to point to the shellcheck-v0.11.0 subdirectory (the extract
directory) rather than using dirname on SHELLCHECK_BIN. This ensures that when
actionlint shells out to shellcheck, it will find the binary in the PATH.
- Around line 144-151: The extraction gate for actionlint uses an unversioned
path `$cache_dir/actionlint` to check whether extraction is needed, which causes
the old binary from a previous version to block extraction of a new version when
ACTIONLINT_VERSION is bumped. To fix this, create a versioned extraction
directory variable (similar to how Shellcheck handles it with extract_dir) that
incorporates ACTIONLINT_VERSION in the path, then update the conditional check
to use this versioned path instead of the unversioned `$cache_dir/actionlint`,
and update the tar extraction command to extract into the versioned directory to
ensure version upgrades trigger proper extraction and checksum verification.
---
Nitpick comments:
In `@tests/test_lib.bats`:
- Around line 163-187: The validate_secrets function appears to have optional
Weblate-related checks that are gated by a parameter, but there is no test
verifying that validate_secrets succeeds when called without the weblate
argument and Weblate environment variables are unset. Add a new test case after
the existing validate_secrets tests that calls validate_secrets with required
variables GITHUB_TOKEN and LANG_CODES set, but with WEBLATE_URL and
WEBLATE_TOKEN explicitly unset, and verify the function returns a successful
exit status of 0. This confirms that Weblate-specific validation is properly
gated by the optional parameter.
- Around line 189-208: Add a new test case after the "validate_secrets weblate:
fails when WEBLATE_URL is unset" test that follows the same pattern but tests
for missing WEBLATE_TOKEN instead. The test should load environment variables,
set GITHUB_TOKEN, LANG_CODES, and WEBLATE_URL, explicitly unset WEBLATE_TOKEN,
run validate_secrets weblate, assert the status is 1, and verify the output
contains the message indicating WEBLATE_TOKEN secret is not set. This provides
symmetric test coverage for both required Weblate configuration parameters.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d565c82d-ee93-4009-bde3-0ac3ef51706c
📒 Files selected for processing (5)
.github/workflows/add-submodules.yml.github/workflows/assets/lib.sh.github/workflows/start-translation.ymlscripts/lint.shtests/test_lib.bats
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/assets/lib.sh
- .github/workflows/add-submodules.yml
2 participants
Close #20.
Summary by CodeRabbit
Release Notes
Chores
Tests