Skip to content

Conversation

@salmanmkc
Copy link
Contributor

Fix broken Node.js upgrade workflow

The automated Node upgrade workflow has been creating useless PRs like this one: #4050

The problem was that it was looking for the wrong release tag format in the alpine_nodejs repo and ended up with empty version strings.

What was broken

The workflow assumed alpine_nodejs releases were tagged like node20-20.19.5 but they're actually just v20.19.5. So when it tried to fall back to alpine releases, the grep patterns failed and we got empty strings.

Result: PRs showing "Node 20: 20.19.5 → " with nothing after the arrow.

What I fixed

  1. Fixed the tag patterns - Changed from looking for ^node20- to ^v20\. which actually matches the real tags
  2. Added validation - Now it checks that versions aren't empty and match the expected x.y.z format before doing anything
  3. Better error handling - If something goes wrong, it shows you what releases are actually available instead of silently failing
  4. Verification - Double-checks that the file updates actually worked

Testing

Ran the grep patterns manually against the real alpine_nodejs releases and confirmed they work:

  • v20.19.5, v24.8.0 etc. are correctly parsed to 20.19.5, 24.8.0
  • Validation correctly rejects empty strings and malformed versions

This should prevent any more broken automation PRs and make debugging easier when things do go wrong.

… The Node.js upgrade workflow was creating broken PRs with empty version numbers due to incorrect tag format assumptions and lack of validation. ## Root Cause - Alpine nodejs releases use 'vX.Y.Z' format, not 'nodeXX-X.Y.Z' format - No validation prevented empty/invalid versions from being used - Silent failures when grep patterns didn't match expected formats ## Changes Made 1. **Fix tag format patterns**: - Change from '^node20-' to '^v20\.' patterns - Change from '^node24-' to '^v24\.' patterns - Correctly extract versions with 'sed s/^v//' instead of 's/^node20-//' 2. **Add comprehensive validation**: - Validate versions are not empty after API calls - Validate versions match x.y.z format using regex '^[0-9]+\.[0-9]+\.[0-9]+$' - Multiple validation checkpoints throughout the workflow - Final validation before creating PR to prevent invalid updates 3. **Improve error handling**: - Display available releases for debugging when validation fails - Clear error messages with actionable information - Fail fast instead of creating broken PRs - Verify file updates were applied correctly 4. **Enhance PR creation**: - Use validated variables instead of GitHub outputs in commit messages - Remove single quotes from here-doc to allow variable expansion - Better verification of sed command results ## Impact - Eliminates invalid PRs with empty version numbers - Future-proof against upstream tag format changes - Better debugging information when issues occur - Maintains data integrity in externals.sh Fixes: Node.js upgrade PRs showing "Node 20: 20.19.5 → (empty)"
@salmanmkc salmanmkc requested a review from a team as a code owner September 23, 2025 03:50
Copilot AI review requested due to automatic review settings September 23, 2025 03:50
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a broken automated Node.js upgrade workflow that was creating PRs with empty version strings due to incorrect release tag pattern matching. The workflow was looking for node20-X.Y.Z format tags but the alpine_nodejs repository actually uses vX.Y.Z format.

Key changes:

  • Fixed grep patterns to match actual alpine_nodejs release tag format (^vX.Y.Z instead of ^nodeX-)
  • Added comprehensive validation to prevent empty or malformed version strings
  • Enhanced error handling with better diagnostics when version detection fails

Comment on lines +50 to +57
if [ -z "$LATEST_NODE20" ] || [ "$LATEST_NODE20" = "" ]; then
echo "::error title=Invalid Node 20 Version::Failed to determine valid Node 20 version. Got: '$LATEST_NODE20'"
echo "Available alpine_nodejs releases:"
echo "$ALPINE_RELEASES" | head -10
exit 1
fi
if [ -z "$LATEST_NODE24" ] || [ "$LATEST_NODE24" = "" ]; then
Copy link

Copilot AI Sep 23, 2025

Choose a reason for hiding this comment

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

The condition [ -z \"$LATEST_NODE20\" ] || [ \"$LATEST_NODE20\" = \"\" ] is redundant. The -z test already checks for empty strings, so the || [ \"$LATEST_NODE20\" = \"\" ] part is unnecessary and can be removed.

Suggested change
if [ -z "$LATEST_NODE20" ] || [ "$LATEST_NODE20" = "" ]; then
echo "::error title=Invalid Node 20 Version::Failed to determine valid Node 20 version. Got: '$LATEST_NODE20'"
echo "Available alpine_nodejs releases:"
echo "$ALPINE_RELEASES" | head -10
exit 1
fi
if [ -z "$LATEST_NODE24" ] || [ "$LATEST_NODE24" = "" ]; then
if [ -z "$LATEST_NODE20" ]; then
echo "::error title=Invalid Node 20 Version::Failed to determine valid Node 20 version. Got: '$LATEST_NODE20'"
echo "Available alpine_nodejs releases:"
echo "$ALPINE_RELEASES" | head -10
exit 1
fi
if [ -z "$LATEST_NODE24" ]; then

Copilot uses AI. Check for mistakes.
Comment on lines +50 to +57
if [ -z "$LATEST_NODE20" ] || [ "$LATEST_NODE20" = "" ]; then
echo "::error title=Invalid Node 20 Version::Failed to determine valid Node 20 version. Got: '$LATEST_NODE20'"
echo "Available alpine_nodejs releases:"
echo "$ALPINE_RELEASES" | head -10
exit 1
fi
if [ -z "$LATEST_NODE24" ] || [ "$LATEST_NODE24" = "" ]; then
Copy link

Copilot AI Sep 23, 2025

Choose a reason for hiding this comment

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

The condition [ -z \"$LATEST_NODE24\" ] || [ \"$LATEST_NODE24\" = \"\" ] is redundant. The -z test already checks for empty strings, so the || [ \"$LATEST_NODE24\" = \"\" ] part is unnecessary and can be removed.

Suggested change
if [ -z "$LATEST_NODE20" ] || [ "$LATEST_NODE20" = "" ]; then
echo "::error title=Invalid Node 20 Version::Failed to determine valid Node 20 version. Got: '$LATEST_NODE20'"
echo "Available alpine_nodejs releases:"
echo "$ALPINE_RELEASES" | head -10
exit 1
fi
if [ -z "$LATEST_NODE24" ] || [ "$LATEST_NODE24" = "" ]; then
if [ -z "$LATEST_NODE20" ]; then
echo "::error title=Invalid Node 20 Version::Failed to determine valid Node 20 version. Got: '$LATEST_NODE20'"
echo "Available alpine_nodejs releases:"
echo "$ALPINE_RELEASES" | head -10
exit 1
fi
if [ -z "$LATEST_NODE24" ]; then

Copilot uses AI. Check for mistakes.
Comment on lines 168 to +173
# Create PR body using here-doc for proper formatting
cat > pr_body.txt << 'EOF'
cat > pr_body.txt << EOF
Automated Node.js version update:
- Node 20: ${{ steps.node-versions.outputs.current_node20 }} → ${{ steps.node-versions.outputs.latest_node20 }}
- Node 24: ${{ steps.node-versions.outputs.current_node24 }} → ${{ steps.node-versions.outputs.latest_node24 }}
- Node 20: ${{ steps.node-versions.outputs.current_node20 }} → $NODE20_VERSION
- Node 24: ${{ steps.node-versions.outputs.current_node24 }} → $NODE24_VERSION
Copy link

Copilot AI Sep 23, 2025

Choose a reason for hiding this comment

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

Changing from << 'EOF' to << EOF enables variable expansion in the heredoc, which is necessary for the variables to be substituted. However, this makes the heredoc vulnerable to shell injection if the variables contain special characters. Consider using proper escaping or validation of the variables before using them in the heredoc.

See below for a potential fix:

          # Sanitize variables to prevent shell injection in heredoc
          safe_NODE20_VERSION=$(printf '%s' "$NODE20_VERSION" | tr -d '\n' | sed 's/"/\\"/g')
          safe_NODE24_VERSION=$(printf '%s' "$NODE24_VERSION" | tr -d '\n' | sed 's/"/\\"/g')

          # Create PR body using here-doc for proper formatting
          cat > pr_body.txt << EOF
          Automated Node.js version update:

          - Node 20: ${{ steps.node-versions.outputs.current_node20 }} → $safe_NODE20_VERSION
          - Node 24: ${{ steps.node-versions.outputs.current_node24 }} → $safe_NODE24_VERSION

Copilot uses AI. Check for mistakes.
@salmanmkc salmanmkc merged commit 338d83a into main Sep 23, 2025
10 checks passed
@salmanmkc salmanmkc deleted the salmanmkc-patch-2 branch September 23, 2025 14:30
fmartinez255 pushed a commit to TiVo/actions-runner that referenced this pull request Oct 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants