chore: Github action release improvements #5066
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughReplaces CircleCI with a deprecated stub and migrates CI/release to GitHub Actions: adds detect/build/sign (macOS, Windows) workflows, a release workflow, many release helper scripts (Bash/PowerShell), a release assets config and library, docs, and .gitignore updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant GH as GitHub Actions
participant Detect as detect_release
participant Build as build
participant SignMac as sign_macos (workflow_call)
participant SignWin as sign_windows (workflow_call)
participant Merge as merge_signed_binaries
participant Release as release.yml
Dev->>GH: push / tag / dispatch
GH->>Detect: run detect_release
Detect-->>GH: outputs is_release
GH->>Build: run build (needs: detect_release)
Build-->>GH: upload build artifacts (unsigned)
alt is_release == true
GH->>SignMac: call sign-macos (needs: build, detect_release)
SignMac-->>GH: upload macos-signed-files
GH->>SignWin: call sign-windows (needs: build, detect_release)
SignWin-->>GH: upload windows-signed-files
GH->>Merge: run merge_signed_binaries (needs: sign_macos, sign_windows)
Merge-->>GH: produce consolidated `bin/` with signed artifacts
end
opt tag push triggers release
GH->>Release: run release.yml (uses build/sign outputs)
Release->>GH: verify release exists, create archives, generate checksums
Release->>GH: upload assets, verify uploads
Release-->>Dev: append upload summary to step summary
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/scripts/release/sign-windows.ps1 (1)
52-52: Add end-of-string anchor to pre-release tag pattern.The regex on line 52 lacks a
$anchor, allowing partial matches likealpha-202511100133extrato succeed. WhileGITHUB_REF_NAMEprovides exact tag names in practice, add the anchor for defensive programming:- elseif ($rawVersion -match '^([a-zA-Z0-9_-]+)-(\d{4})(\d{2})(\d{2})(\d{2})') { + elseif ($rawVersion -match '^([a-zA-Z0-9_-]+)-(\d{4})(\d{2})(\d{2})(\d{2})$') {
🧹 Nitpick comments (2)
.github/scripts/release/verify-binaries-downloaded.sh (1)
16-22: Add robustness for edge cases when sourcing configuration.If the configuration file is malformed or
get_binary_countreturns a non-numeric value, the arithmetic comparison on line 40 will silently fail or produce unexpected behavior. Additionally, whileset -ehandles a failedverify_config_filecall, adding an explicit check would clarify the intent.Apply this diff to add explicit error handling:
if [[ -n "${2:-}" ]]; then expected_count="$2" else - verify_config_file - expected_count=$(get_binary_count) + verify_config_file || { + echo "ERROR: Unable to verify configuration file" >&2 + exit 1 + } + expected_count=$(get_binary_count) || { + echo "ERROR: Failed to retrieve binary count from configuration" >&2 + exit 1 + } + if ! [[ "$expected_count" =~ ^[0-9]+$ ]]; then + echo "ERROR: Expected count is not a valid number: $expected_count" >&2 + exit 1 + fi fi.github/scripts/release/README.md (1)
374-374: Consider minor wording adjustment for clarity.The phrase "ready for use" is idiomatic, though "ready to use" may flow slightly better in this context.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.github/assets/release-assets-config.json(1 hunks).github/scripts/release/README.md(1 hunks).github/scripts/release/check-release-exists.sh(1 hunks).github/scripts/release/generate-upload-summary.sh(1 hunks).github/scripts/release/lib-release-config.sh(1 hunks).github/scripts/release/restore-p12-certificate.ps1(1 hunks).github/scripts/release/set-permissions.sh(1 hunks).github/scripts/release/sign-windows.ps1(1 hunks).github/scripts/release/verify-assets-uploaded.sh(1 hunks).github/scripts/release/verify-binaries-downloaded.sh(1 hunks).github/workflows/sign-windows.yml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .github/assets/release-assets-config.json
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/scripts/release/generate-upload-summary.sh
- .github/scripts/release/check-release-exists.sh
- .github/scripts/release/restore-p12-certificate.ps1
🧰 Additional context used
🧬 Code graph analysis (3)
.github/scripts/release/verify-binaries-downloaded.sh (1)
.github/scripts/release/lib-release-config.sh (2)
verify_config_file(73-78)get_binary_count(14-16)
.github/scripts/release/set-permissions.sh (1)
.github/scripts/release/lib-release-config.sh (2)
verify_config_file(73-78)get_all_binaries(9-11)
.github/scripts/release/verify-assets-uploaded.sh (2)
.github/scripts/release/upload-assets.sh (2)
main(12-47)assert_env_var_not_empty(49-57).github/scripts/release/lib-release-config.sh (4)
verify_config_file(73-78)get_all_expected_files(34-52)get_total_file_count(19-21)get_binary_count(14-16)
🪛 LanguageTool
.github/scripts/release/README.md
[uncategorized] ~374-~374: The preposition ‘to’ seems more likely in this position.
Context: ... smctl version - Confirms tool is ready for use ### restore-p12-certificate.ps1 ...
(AI_HYDRA_LEO_REPLACE_FOR_TO)
🪛 Shellcheck (0.11.0)
.github/scripts/release/lib-release-config.sh
[warning] 35-35: archive_ext appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 35-35: additional appears unused. Verify use (or export if used externally).
(SC2034)
⏰ 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). (2)
- GitHub Check: license_check / License Check
- GitHub Check: Pull Request has non-contributor approval
🔇 Additional comments (4)
.github/scripts/release/set-permissions.sh (1)
1-41: LGTM!The script follows bash best practices with proper error handling (
set -e), input validation, configuration-driven binary enumeration viaget_all_binaries(), and graceful handling of missing files with clear warnings..github/scripts/release/verify-assets-uploaded.sh (1)
1-118: LGTM!The verification logic is robust with proper retry handling (MAX_RETRIES=10), clear error messages, and a sensible spot-check for asset downloadability. The configuration-driven approach via
get_all_expected_files()ensures consistency across the release tooling..github/scripts/release/sign-windows.ps1 (1)
328-368: LGTM!The signing flow correctly handles both Windows binaries (386 and amd64) with proper validation, signing, and verification for each. The implementation properly addresses the PR objective of adding Windows executable signing.
.github/scripts/release/lib-release-config.sh (1)
33-52: LGTM!The configuration-driven approach via
jqqueries against a centralized JSON file ensures consistency across all release scripts. The static analysis warning about unusedarchive_extandadditionalvariables on line 35 is a false positive—the function correctly uses command substitution directly in the loops.
| echo "Downloaded binaries:" | ||
| ls -lahrt "$bin_dir"/* | ||
|
|
||
| # Count binaries | ||
| local binary_count | ||
| binary_count=$(find "$bin_dir/" -type f | wc -l) | ||
| echo "Total binaries: $binary_count" |
There was a problem hiding this comment.
Clarify the scope of binary counting to match the ls output.
Lines 30 and 34 have a subtle inconsistency:
- Line 30 lists directory contents with
ls "$bin_dir"/*, which includes both files and subdirectories at the top level - Line 34 counts with
find "$bin_dir/" -type f, which recursively counts only regular files
If the bin_dir contains subdirectories with binaries, the user will see directory entries in the ls output but those directories won't be counted in the final total. This creates confusion about what "Total binaries" actually represents.
Decide whether binaries should be:
- Only at the top level (non-recursive) – then use
find "$bin_dir/" -maxdepth 1 -type f | wc -l - Recursive throughout the tree – then adjust the
lscommand tofind "$bin_dir/" -type f -lsor removelsentirely
🤖 Prompt for AI Agents
In .github/scripts/release/verify-binaries-downloaded.sh around lines 29 to 35,
the listing uses ls "$bin_dir"/* (which shows top-level files and directories)
while the count uses find "$bin_dir/" -type f (recursive), causing a mismatch;
make them consistent by replacing the ls line with a recursive file listing
using find "$bin_dir/" -type f -ls (or similar find output) and keep the
binary_count using find "$bin_dir/" -type f | wc -l so both the displayed
entries and the counted total reflect the same recursive scope.
denis256
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/assets/release-assets-config.json(1 hunks).github/scripts/release/README.md(1 hunks).github/scripts/release/sign-windows.ps1(1 hunks)
🧰 Additional context used
🪛 LanguageTool
.github/scripts/release/README.md
[uncategorized] ~430-~430: The official name of this software platform is spelled with a capital “H”.
Context: ...ion-Driven Behavior:** The script reads .github/assets/release-assets-config.json to d...
(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). (2)
- GitHub Check: license_check / License Check
- GitHub Check: Pull Request has non-contributor approval
🔇 Additional comments (16)
.github/scripts/release/README.md (4)
1-13: Excellent documentation structure and organization.The README provides comprehensive coverage of the release infrastructure with clear sections, practical examples, and proper organization. The use of subsections (Core Library, General Release Scripts, macOS Signing Scripts, Windows Signing Scripts) and consistent formatting makes the documentation easy to navigate.
43-81: Configuration schema is clear and well-documented.The centralized configuration schema is properly explained with a valid JSON example showing all required fields. The description of how scripts consume the configuration via
lib-release-config.shhelps readers understand the integration pattern.
127-269: Script documentation is thorough with consistent formatting.Each script includes purpose, environment variables, usage examples, and output/features descriptions. This consistency and detail level will help maintainers and contributors understand how to use and extend these scripts. Environment variable documentation is particularly helpful for operational use.
522-531: Security notes are comprehensive and follow best practices.The documentation properly covers input validation, proper quoting, secrets handling via environment variables only, and fail-fast error strategies. These guidelines appropriately emphasize defensive coding patterns critical for release automation.
.github/assets/release-assets-config.json (2)
1-45: Platform configuration is well-structured with appropriate signing strategy.The seven platforms are properly defined with consistent naming, correct
.exeextensions for Windows binaries, and a reasonable signing strategy:
- macOS (both architectures): signed ✓
- Linux (all architectures): unsigned ✓
- Windows 386: unsigned ✓
- Windows amd64: signed ✓
This selective signing approach appears optimal for balancing security (amd64 is most common) with resource constraints (DigiCert signatures have costs/quotas).
62-68: Count totals are arithmetically correct and consistent with documentation.Verified: 7 binaries + (7 × 2 archive formats) + 1 SHA256SUMS = 22 total files. These counts match the expectations documented in README.md lines 249 and 74-78. The hardcoded counts require manual updates if platforms are added, but this is acceptable for a static configuration file used in release workflows.
Confirm that the actual release workflow generates exactly 22 files matching this configuration (7 binaries + 7 zips + 7 tar.gz archives + 1 SHA256SUMS file), and that any script consuming this config validates the counts on each run.
.github/scripts/release/sign-windows.ps1 (10)
1-29: LGTM: Script header and environment validation are well-structured.The documentation clearly describes usage and required environment variables, and the Assert-EnvVar function properly validates required environment variables with clear error messages.
31-51: LGTM: Platform configuration loading is robust.The function properly validates the config file exists, filters Windows platforms, and ensures at least one platform is found before proceeding.
53-219: LGTM: Version parsing and resource generation are comprehensive.The function handles multiple tag formats (standard, pre-release, dev) with appropriate fallbacks, generates complete Windows resource metadata, and validates the icon file exists. The version parsing logic correctly ensures all components stay within Windows limits (65535).
221-251: LGTM: Binary patching with proper error handling.The function correctly adds Go bin to PATH, validates each binary exists before patching, and properly handles patch failures. The PATH modification is acceptable for script-scoped execution.
266-277: LGTM: Healthcheck appropriately treats failures as non-critical.The function correctly continues on healthcheck failure with a warning, relying on the signing step as the definitive validation. This is appropriate for operational resilience.
279-290: LGTM: Certificate synchronization with proper error handling.The function correctly syncs certificates from DigiCert KeyLocker and fails fast with a clear error message if synchronization fails.
292-309: LGTM: Binary signing with robust error handling.The function properly signs binaries with verbose output and fails immediately if signing fails, ensuring no unsigned binaries proceed through the pipeline.
325-394: LGTM: Well-orchestrated main execution flow.The Main function properly validates prerequisites, executes steps in logical order, and uses config-driven signing decisions for each platform. The summary output clearly reports signed vs. patched counts, and the configuration-based approach allows flexible per-platform control.
311-323: Signature verification should fail, not warn.The verify step only emits a warning on non-zero exit code, which undermines the security guarantee. SMCTL allows users to easily sign files using either a keypair alias or certificate fingerprint and do things like verify digital signatures, but you can verify your signature using either the smctl sign verify command or Windows SignTool. The code's "may be expected" comment suggests unreliable exit codes, yet verification should still be enforced.
Either document why smctl verify failures are acceptable, or change lines 318–321 to fail the build on verification errors to ensure only verifiably signed binaries are released. You can manually verify a script's signature by using the Get-AuthenticodeSignature cmdlet in PowerShell or through the properties of the file in Windows—consider this native alternative if smctl's exit codes are unreliable.
253-264: Rewritten Review Comment: smctl credentials save follows DigiCert's documented patternDigiCert's official documentation shows
smctl credentials saveaccepts API token and certificate password as command-line arguments by design. This is the documented and only supported method—no alternative input mechanisms (stdin, file-based, prompting) are provided.The credentials are saved to the operating system credential store, which is the secure destination. While CLI arguments are briefly visible during the one-time save operation, this is: (1) a constraint of the smctl tool, not the repository code; (2) a standard pattern for DigiCert KeyLocker integration in CI/CD; and (3) mitigated by GitHub Actions' secret masking and controlled execution environment.
The code is correct and follows DigiCert's best practices for this integration.
| The scripts support multiple version tag formats: | ||
|
|
||
| | Format | Example | Windows FileVersion | Description | | ||
| |--------|---------|---------------------|-------------| | ||
| | Standard | `v0.93.4` | `0.93.4.0` | Semantic version with v prefix | | ||
| | Beta | `beta-2025111001` | `2025.1110.01.0` | Pre-release with date timestamp | | ||
| | Alpha | `alpha-2025110301` | `2025.1103.01.0` | Alpha with date timestamp | | ||
| | Generic | `<prefix>-YYYYMMDDNN` | `YYYY.MMDD.NN.0` | Any prefix with date | | ||
|
|
||
| **Windows Version Constraints:** | ||
| - Each component must be ≤ 65535 | ||
| - Format: YYYY.MMDD.NN.0 keeps all components within limits |
There was a problem hiding this comment.
🧩 Analysis chain
Version format support table is clear but verify implementation alignment.
The table clearly documents version format transformations (e.g., beta-2025111001 → 2025.1110.01.0). Ensure the actual implementation in sign-windows.ps1 and related scripts matches these documented transformations exactly.
To verify the version format logic matches the documentation:
🏁 Script executed:
#!/bin/bash
# Search for version parsing logic in Windows signing script
rg -A 10 -B 5 "version.*format|beta-|alpha-|YYYYMMDD" .github/scripts/release/*.ps1 .github/scripts/release/*.shLength of output: 4527
I need to see the complete version string construction logic in sign-windows.ps1. Let me retrieve the full section that builds the version number.
---
🏁 Script executed:
```shell
#!/bin/bash
# Get the complete version parsing and construction logic from sign-windows.ps1
rg -A 30 "Generic pre-release tag:" .github/scripts/release/sign-windows.ps1
Length of output: 1351
Let me verify where these parsed version components are used to patch the actual FileVersion:
---
🏁 Script executed:
```shell
#!/bin/bash
# Find where the $major, $minor, $patch, $build variables are used to set FileVersion
rg -A 5 "FileVersion|ProductVersion" .github/scripts/release/sign-windows.ps1 | head -50
Length of output: 1016
FileVersion format implementation does not match documented transformation for pre-release versions.
The code correctly parses version components but assigns $version (containing the prefix, e.g., beta-2025111001) to FileVersion instead of the numeric format. The comment states "FileVersion will be 2025.1110.01.0" but the actual assignment uses FileVersion = $version, which produces an invalid Windows version string.
Fix: In sign-windows.ps1, the pre-release version block should construct the numeric FileVersion: $fileVersion = "$major.$minor.$patch.0" and use that instead of $version for the FileVersion and ProductVersion assignments.
🤖 Prompt for AI Agents
.github/scripts/release/README.md around lines 509-520: the README notes that
pre-release tags (e.g., beta-2025111001) should be transformed to a numeric
Windows FileVersion (e.g., 2025.1110.01.0) but the implementation in
sign-windows.ps1 currently assigns the raw $version string to
FileVersion/ProductVersion. Update the pre-release handling in sign-windows.ps1
to parse out $major, $minor, $patch from the date-based tag, build a numeric
$fileVersion string like "$major.$minor.$patch.0" (ensuring each component ≤
65535), and use that $fileVersion for both FileVersion and ProductVersion
assignments instead of $version.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/assets/release-assets-config.json (1)
62-68: Consider computingcountsdynamically instead of manual maintenance.The
countsobject contains manually maintained aggregates (binaries, archives_per_format, total, etc.) that mirror the structure of the platforms and formats arrays. This creates a maintenance burden: if platforms or formats are added/removed, counts must be kept in sync or the configuration becomes stale and inconsistent.If these counts are used by release scripts, consider computing them dynamically from the arrays instead of hard-coding them here.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/assets/release-assets-config.json(1 hunks)
⏰ 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). (4)
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: license_check / License Check
- GitHub Check: Pull Request has non-contributor approval
🔇 Additional comments (1)
.github/assets/release-assets-config.json (1)
33-37: Verify intent behind unsigned Windows 386 binary.Line 36 marks Windows 386 as
"signed": falsewhile Windows amd64 (line 42) is signed. This asymmetry in signing strategy across Windows architectures is unusual and warrants clarification—is this intentional (e.g., 386 being phased out) or an oversight?
| exit 1 | ||
| fi | ||
|
|
||
| cd "$bin_dir" |
There was a problem hiding this comment.
NIT: Consider using pushd in scripts instead of cd unless you want the caller to have a different working directory. It avoids bugs that can arise from side effects when calling a script.
| echo "ERROR: INPUT_TAG is empty for workflow_dispatch event" >&2 | ||
| exit 1 | ||
| fi | ||
| else |
There was a problem hiding this comment.
NIT: This is me being neurotic, but I usually consider an else a smell. We can have a resolve_version function or something and return from that with an early return to clean up this section.
| "description": "Checksums for all files" | ||
| } | ||
| ], | ||
| "counts": { |
There was a problem hiding this comment.
NIT: I think these values should be computed so that we can't forget to update them later if we add a new platform. I don't think making that change should block merging this PR, though.
| echo "Upload phase completed" | ||
| } | ||
|
|
||
| function assert_env_var_not_empty { |
There was a problem hiding this comment.
NIT: You copy + paste this around. You could move this to a lib as well.
There was a problem hiding this comment.
FYI this is a short way to make the assertion:
: ${VAR:?ERROR: VAR is a required environment variable}"
| assets=$(gh release view "$VERSION" --json 'assets' --jq '.assets[].name') | ||
|
|
||
| local asset_count | ||
| asset_count=$(echo "$assets" | wc -l) |
| echo "Checking $expected_file..." | ||
|
|
||
| # Check if file exists in release | ||
| if ! echo "$assets" | grep -q "^${expected_file}$"; then |
| if gh release upload "$VERSION" "$bin_dir/$expected_file" $clobber_flag; then | ||
| echo "Uploaded $expected_file" | ||
| break | ||
| else |
There was a problem hiding this comment.
NIT: We don't need this else.
|
|
||
| # Get expected count from configuration, or use parameter if provided | ||
| local expected_count | ||
| if [[ -n "${2:-}" ]]; then |
There was a problem hiding this comment.
NIT: Avoid leveraging parameters here like this. We should give it a name at the entry to the function. Also, we can have expected count be a function to avoid this else.
| echo "Verifying required files..." | ||
|
|
||
| # Check macOS binaries | ||
| for file in terragrunt_darwin_amd64 terragrunt_darwin_arm64; do |
There was a problem hiding this comment.
NIT: We can compute this off the JSON file using the os field.
Description
Example artifacts publication:
https://github.com/gruntwork-io/terragrunt/releases/tag/beta-2025111005
https://github.com/gruntwork-io/terragrunt/releases/tag/alpha-2025111102
TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added / Removed / Updated [X].
Migration Guide
Summary by CodeRabbit
New Features
Chores
Documentation