-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Merge Maintenance 9.x to master, mostly for the github workflow #11273
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Add Ray Morris (Sensei) to AUTHORS
Create automated check that runs on PRs to detect when parameter group structs are modified without incrementing the version number. This prevents settings corruption issues like those seen in PR #11236. Detection logic: - Scans changed C/H files for PG_REGISTER entries - Detects struct typedef modifications in git diff - Verifies version parameter was incremented - Posts helpful comment if version not incremented Files added: - .github/scripts/check-pg-versions.sh - Detection script - .github/workflows/pg-version-check.yml - Workflow definition - .github/workflows/README.md - Workflow documentation The check is non-blocking (comment only) to avoid false positive build failures, but provides clear guidance on when versions must be incremented.
Improve PG version check robustness and efficiency per Qodo feedback: 1. Struct detection: Use sed/grep pipeline to better filter comments and whitespace, reducing false positives. Isolates struct body boundaries more reliably before checking for modifications. 2. File iteration: Use while/read loop instead of for loop to correctly handle filenames that contain spaces. 3. Workflow efficiency: Capture script output once and pass between steps using GITHUB_OUTPUT multiline strings, eliminating redundant execution. Changes reduce false positives and improve compatibility while maintaining the same detection logic.
Add GitHub Action to detect Parameter Group version increment issues
PR Compliance Guide 🔍All compliance sections have been disabled in the configurations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
High-level Suggestion
Replace the fragile sed/grep shell script logic for parsing C code with a more robust solution. Utilize a proper C parser, such as clang-query or pycparser, to analyze the code's Abstract Syntax Tree (AST) for changes, which will improve accuracy and reduce errors. [High-level, importance: 9]
Solution Walkthrough:
Before:
```bash
# .github/scripts/check-pg-versions.sh
# Get diff for a file
diff_output=$(git diff ... -- "$file")
# Extract PG_REGISTER line with regex
pg_registers=$(echo "$diff_output" | grep "PG_REGISTER")
# For each registration...
# Extract struct name with regex
[[ $pg_line =~ PG_REGISTER... ]]
struct_type="${BASH_REMATCH[1]}"
# Find struct definition in diff using another regex
struct_pattern="typedef struct ${struct_type%_t}_s"
struct_body_diff=$(echo "$diff_output" | sed -n "/${struct_pattern}/,/\}.*${struct_type};/p")
# Check for changes in struct body by stripping comments/whitespace
struct_changes=$(echo "$struct_body_diff" | grep ... | sed ... | tr ...)
if [ -n "$struct_changes" ]; then
# Check if version was incremented, again using grep/regex
...
fi
#### After:
```bash
```python
# Conceptual Python script using a C parser
import pycparser
from pycparser import c_ast
# Get old and new versions of the file from git
# Parse old and new versions of the file into an AST
ast_old = pycparser.parse_file('old_file.c', use_cpp=True)
ast_new = pycparser.parse_file('new_file.c', use_cpp=True)
# Visitor to find PG_REGISTER calls and struct definitions
class PGVisitor(c_ast.NodeVisitor):
def visit_FuncCall(self, node):
# Find PG_REGISTER(...) calls and store struct name/version
...
def visit_Typedef(self, node):
# Find typedef struct ... and store the struct's AST node
...
# Compare struct definitions from old and new ASTs.
# If a struct's AST has changed but its version in PG_REGISTER has not,
# report an issue.
| } catch (err) { | ||
| core.setFailed(`Failed to post comment: ${err}`); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Replace core.setFailed(...) with throw new Error(...) in the actions/github-script step, as the core object is not available in that context and would cause a reference error. [general, importance: 8]
| } catch (err) { | |
| core.setFailed(`Failed to post comment: ${err}`); | |
| } | |
| } catch (err) { | |
| throw new Error(`Failed to post comment: ${err}`); | |
| } |
| - name: Run PG version check script | ||
| id: pg_check | ||
| run: | | ||
| set +e # Don't fail the workflow, just capture exit code | ||
| # Run script and capture output. Exit code 1 is expected for issues. | ||
| # The output is captured and encoded to be passed between steps. | ||
| output=$(bash .github/scripts/check-pg-versions.sh 2>&1) | ||
| exit_code=$? | ||
| echo "exit_code=${exit_code}" >> $GITHUB_OUTPUT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Add a step to the workflow to explicitly fail the job if the check script returns an exit code of 2, ensuring that script errors are not ignored. [possible issue, importance: 7]
| - name: Run PG version check script | |
| id: pg_check | |
| run: | | |
| set +e # Don't fail the workflow, just capture exit code | |
| # Run script and capture output. Exit code 1 is expected for issues. | |
| # The output is captured and encoded to be passed between steps. | |
| output=$(bash .github/scripts/check-pg-versions.sh 2>&1) | |
| exit_code=$? | |
| echo "exit_code=${exit_code}" >> $GITHUB_OUTPUT | |
| - name: Fail on script errors | |
| if: steps.pg_check.outputs.exit_code == '2' | |
| run: | | |
| echo "::error::PG version check script failed with exit code 2" | |
| exit 2 |
| ISSUES_FILE=$(mktemp) | ||
| trap "rm -f $ISSUES_FILE" EXIT | ||
|
|
||
| # Color output for local testing | ||
| if [ -t 1 ]; then | ||
| RED='\033[0;31m' | ||
| GREEN='\033[0;32m' | ||
| YELLOW='\033[1;33m' | ||
| NC='\033[0m' # No Color | ||
| else | ||
| RED='' | ||
| GREEN='' | ||
| YELLOW='' | ||
| NC='' | ||
| fi | ||
|
|
||
| echo "🔍 Checking for Parameter Group version updates..." | ||
|
|
||
| # Get base and head commits | ||
| BASE_REF=${GITHUB_BASE_REF:-} | ||
| HEAD_REF=${GITHUB_HEAD_REF:-} | ||
|
|
||
| if [ -z "$BASE_REF" ] || [ -z "$HEAD_REF" ]; then | ||
| echo "⚠️ Warning: Not running in GitHub Actions PR context" | ||
| echo "Using git diff against HEAD~1 for local testing" | ||
| BASE_COMMIT="HEAD~1" | ||
| HEAD_COMMIT="HEAD" | ||
| else | ||
| BASE_COMMIT="origin/$BASE_REF" | ||
| HEAD_COMMIT="HEAD" | ||
| fi | ||
|
|
||
| # Get list of changed files | ||
| CHANGED_FILES=$(git diff --name-only $BASE_COMMIT..$HEAD_COMMIT | grep -E '\.(c|h)$' || true) | ||
|
|
||
| if [ -z "$CHANGED_FILES" ]; then | ||
| echo "✅ No C/H files changed" | ||
| exit 0 | ||
| fi | ||
|
|
||
| echo "📁 Changed files:" | ||
| echo "$CHANGED_FILES" | sed 's/^/ /' | ||
|
|
||
| # Function to extract PG info from a file | ||
| check_file_for_pg_changes() { | ||
| local file=$1 | ||
| local diff_output=$(git diff $BASE_COMMIT..$HEAD_COMMIT -- "$file") | ||
|
|
||
| # Check if file contains PG_REGISTER (in either old or new version) | ||
| if ! echo "$diff_output" | grep -q "PG_REGISTER"; then | ||
| return 0 | ||
| fi | ||
|
|
||
| echo " 🔎 Checking $file (contains PG_REGISTER)" | ||
|
|
||
| # Extract all PG_REGISTER lines from the diff (both old and new) | ||
| local pg_registers=$(echo "$diff_output" | grep -E "^[-+].*PG_REGISTER" || true) | ||
|
|
||
| if [ -z "$pg_registers" ]; then | ||
| # PG_REGISTER exists but wasn't changed | ||
| # Still need to check if the struct changed | ||
| pg_registers=$(git show $HEAD_COMMIT:"$file" | grep "PG_REGISTER" || true) | ||
| fi | ||
|
|
||
| # Process each PG registration | ||
| while IFS= read -r pg_line; do | ||
| [ -z "$pg_line" ] && continue | ||
|
|
||
| # Extract struct name and version | ||
| # Pattern: PG_REGISTER.*\((\w+),\s*(\w+),\s*PG_\w+,\s*(\d+)\) | ||
| if [[ $pg_line =~ PG_REGISTER[^(]*\(([^,]+),([^,]+),([^,]+),([^)]+)\) ]]; then | ||
| local struct_type="${BASH_REMATCH[1]}" | ||
| local pg_name="${BASH_REMATCH[2]}" | ||
| local pg_id="${BASH_REMATCH[3]}" | ||
| local version="${BASH_REMATCH[4]}" | ||
|
|
||
| # Clean up whitespace | ||
| struct_type=$(echo "$struct_type" | xargs) | ||
| version=$(echo "$version" | xargs) | ||
|
|
||
| echo " 📋 Found: $struct_type (version $version)" | ||
|
|
||
| # Check if this struct's typedef was modified | ||
| local struct_pattern="typedef struct ${struct_type%_t}_s" | ||
| # Isolate struct body from diff, remove comments and empty lines, then check for remaining changes | ||
| local struct_body_diff=$(echo "$diff_output" | sed -n "/${struct_pattern}/,/\}.*${struct_type};/p") | ||
| local struct_changes=$(echo "$struct_body_diff" | grep -E "^[-+]" \ | ||
| | grep -v -E "^[-+]\s*(typedef struct|}|//|\*)" \ | ||
| | sed -E 's://.*$::' \ | ||
| | sed -E 's:/\*.*\*/::' \ | ||
| | tr -d '[:space:]') | ||
|
|
||
| if [ -n "$struct_changes" ]; then | ||
| echo " ⚠️ Struct definition modified" | ||
|
|
||
| # Check if version was incremented in this diff | ||
| local old_version=$(echo "$diff_output" | grep "^-.*PG_REGISTER.*$struct_type" | grep -oP ',\s*\K\d+(?=\s*\))' || echo "") | ||
| local new_version=$(echo "$diff_output" | grep "^+.*PG_REGISTER.*$struct_type" | grep -oP ',\s*\K\d+(?=\s*\))' || echo "") | ||
|
|
||
| if [ -n "$old_version" ] && [ -n "$new_version" ]; then | ||
| if [ "$new_version" -le "$old_version" ]; then | ||
| echo " ❌ Version NOT incremented ($old_version → $new_version)" | ||
|
|
||
| # Find line number of PG_REGISTER | ||
| local line_num=$(git show $HEAD_COMMIT:"$file" | grep -n "PG_REGISTER.*$struct_type" | cut -d: -f1 | head -1) | ||
|
|
||
| # Add to issues list | ||
| cat >> $ISSUES_FILE << EOF | ||
| ### \`$struct_type\` ($file:$line_num) | ||
| - **Struct modified:** Field changes detected | ||
| - **Version status:** ❌ Not incremented (version $version) | ||
| - **Recommendation:** Review changes and increment version if needed | ||
|
|
||
| EOF | ||
| else | ||
| echo " ✅ Version incremented ($old_version → $new_version)" | ||
| fi | ||
| elif [ -z "$old_version" ] || [ -z "$new_version" ]; then | ||
| # Couldn't determine version change, but struct was modified | ||
| echo " ⚠️ Could not determine if version was incremented" | ||
|
|
||
| local line_num=$(git show $HEAD_COMMIT:"$file" | grep -n "PG_REGISTER.*$struct_type" | cut -d: -f1 | head -1) | ||
|
|
||
| cat >> $ISSUES_FILE << EOF | ||
| ### \`$struct_type\` ($file:$line_num) | ||
| - **Struct modified:** Field changes detected | ||
| - **Version status:** ⚠️ Unable to verify version increment | ||
| - **Current version:** $version | ||
| - **Recommendation:** Verify version was incremented if struct layout changed | ||
|
|
||
| EOF | ||
| fi | ||
| else | ||
| echo " ✅ Struct unchanged" | ||
| fi | ||
| fi | ||
| done <<< "$pg_registers" | ||
| } | ||
|
|
||
| # Check each changed file | ||
| while IFS= read -r file; do | ||
| check_file_for_pg_changes "$file" | ||
| done <<< "$CHANGED_FILES" | ||
|
|
||
| # Check if any issues were found | ||
| if [ -s $ISSUES_FILE ]; then | ||
| echo "" | ||
| echo "${YELLOW}⚠️ Potential PG version issues detected${NC}" | ||
| echo "Output saved to: $ISSUES_FILE" | ||
| cat $ISSUES_FILE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Quote variable expansions (e.g., "$ISSUES_FILE", "$BASE_COMMIT..$HEAD_COMMIT") to avoid failures with whitespace/globs and to make cleanup robust. [Learned best practice, importance: 5]
| ISSUES_FILE=$(mktemp) | |
| trap "rm -f $ISSUES_FILE" EXIT | |
| # Color output for local testing | |
| if [ -t 1 ]; then | |
| RED='\033[0;31m' | |
| GREEN='\033[0;32m' | |
| YELLOW='\033[1;33m' | |
| NC='\033[0m' # No Color | |
| else | |
| RED='' | |
| GREEN='' | |
| YELLOW='' | |
| NC='' | |
| fi | |
| echo "🔍 Checking for Parameter Group version updates..." | |
| # Get base and head commits | |
| BASE_REF=${GITHUB_BASE_REF:-} | |
| HEAD_REF=${GITHUB_HEAD_REF:-} | |
| if [ -z "$BASE_REF" ] || [ -z "$HEAD_REF" ]; then | |
| echo "⚠️ Warning: Not running in GitHub Actions PR context" | |
| echo "Using git diff against HEAD~1 for local testing" | |
| BASE_COMMIT="HEAD~1" | |
| HEAD_COMMIT="HEAD" | |
| else | |
| BASE_COMMIT="origin/$BASE_REF" | |
| HEAD_COMMIT="HEAD" | |
| fi | |
| # Get list of changed files | |
| CHANGED_FILES=$(git diff --name-only $BASE_COMMIT..$HEAD_COMMIT | grep -E '\.(c|h)$' || true) | |
| if [ -z "$CHANGED_FILES" ]; then | |
| echo "✅ No C/H files changed" | |
| exit 0 | |
| fi | |
| echo "📁 Changed files:" | |
| echo "$CHANGED_FILES" | sed 's/^/ /' | |
| # Function to extract PG info from a file | |
| check_file_for_pg_changes() { | |
| local file=$1 | |
| local diff_output=$(git diff $BASE_COMMIT..$HEAD_COMMIT -- "$file") | |
| # Check if file contains PG_REGISTER (in either old or new version) | |
| if ! echo "$diff_output" | grep -q "PG_REGISTER"; then | |
| return 0 | |
| fi | |
| echo " 🔎 Checking $file (contains PG_REGISTER)" | |
| # Extract all PG_REGISTER lines from the diff (both old and new) | |
| local pg_registers=$(echo "$diff_output" | grep -E "^[-+].*PG_REGISTER" || true) | |
| if [ -z "$pg_registers" ]; then | |
| # PG_REGISTER exists but wasn't changed | |
| # Still need to check if the struct changed | |
| pg_registers=$(git show $HEAD_COMMIT:"$file" | grep "PG_REGISTER" || true) | |
| fi | |
| # Process each PG registration | |
| while IFS= read -r pg_line; do | |
| [ -z "$pg_line" ] && continue | |
| # Extract struct name and version | |
| # Pattern: PG_REGISTER.*\((\w+),\s*(\w+),\s*PG_\w+,\s*(\d+)\) | |
| if [[ $pg_line =~ PG_REGISTER[^(]*\(([^,]+),([^,]+),([^,]+),([^)]+)\) ]]; then | |
| local struct_type="${BASH_REMATCH[1]}" | |
| local pg_name="${BASH_REMATCH[2]}" | |
| local pg_id="${BASH_REMATCH[3]}" | |
| local version="${BASH_REMATCH[4]}" | |
| # Clean up whitespace | |
| struct_type=$(echo "$struct_type" | xargs) | |
| version=$(echo "$version" | xargs) | |
| echo " 📋 Found: $struct_type (version $version)" | |
| # Check if this struct's typedef was modified | |
| local struct_pattern="typedef struct ${struct_type%_t}_s" | |
| # Isolate struct body from diff, remove comments and empty lines, then check for remaining changes | |
| local struct_body_diff=$(echo "$diff_output" | sed -n "/${struct_pattern}/,/\}.*${struct_type};/p") | |
| local struct_changes=$(echo "$struct_body_diff" | grep -E "^[-+]" \ | |
| | grep -v -E "^[-+]\s*(typedef struct|}|//|\*)" \ | |
| | sed -E 's://.*$::' \ | |
| | sed -E 's:/\*.*\*/::' \ | |
| | tr -d '[:space:]') | |
| if [ -n "$struct_changes" ]; then | |
| echo " ⚠️ Struct definition modified" | |
| # Check if version was incremented in this diff | |
| local old_version=$(echo "$diff_output" | grep "^-.*PG_REGISTER.*$struct_type" | grep -oP ',\s*\K\d+(?=\s*\))' || echo "") | |
| local new_version=$(echo "$diff_output" | grep "^+.*PG_REGISTER.*$struct_type" | grep -oP ',\s*\K\d+(?=\s*\))' || echo "") | |
| if [ -n "$old_version" ] && [ -n "$new_version" ]; then | |
| if [ "$new_version" -le "$old_version" ]; then | |
| echo " ❌ Version NOT incremented ($old_version → $new_version)" | |
| # Find line number of PG_REGISTER | |
| local line_num=$(git show $HEAD_COMMIT:"$file" | grep -n "PG_REGISTER.*$struct_type" | cut -d: -f1 | head -1) | |
| # Add to issues list | |
| cat >> $ISSUES_FILE << EOF | |
| ### \`$struct_type\` ($file:$line_num) | |
| - **Struct modified:** Field changes detected | |
| - **Version status:** ❌ Not incremented (version $version) | |
| - **Recommendation:** Review changes and increment version if needed | |
| EOF | |
| else | |
| echo " ✅ Version incremented ($old_version → $new_version)" | |
| fi | |
| elif [ -z "$old_version" ] || [ -z "$new_version" ]; then | |
| # Couldn't determine version change, but struct was modified | |
| echo " ⚠️ Could not determine if version was incremented" | |
| local line_num=$(git show $HEAD_COMMIT:"$file" | grep -n "PG_REGISTER.*$struct_type" | cut -d: -f1 | head -1) | |
| cat >> $ISSUES_FILE << EOF | |
| ### \`$struct_type\` ($file:$line_num) | |
| - **Struct modified:** Field changes detected | |
| - **Version status:** ⚠️ Unable to verify version increment | |
| - **Current version:** $version | |
| - **Recommendation:** Verify version was incremented if struct layout changed | |
| EOF | |
| fi | |
| else | |
| echo " ✅ Struct unchanged" | |
| fi | |
| fi | |
| done <<< "$pg_registers" | |
| } | |
| # Check each changed file | |
| while IFS= read -r file; do | |
| check_file_for_pg_changes "$file" | |
| done <<< "$CHANGED_FILES" | |
| # Check if any issues were found | |
| if [ -s $ISSUES_FILE ]; then | |
| echo "" | |
| echo "${YELLOW}⚠️ Potential PG version issues detected${NC}" | |
| echo "Output saved to: $ISSUES_FILE" | |
| cat $ISSUES_FILE | |
| ISSUES_FILE="$(mktemp)" | |
| trap 'rm -f "$ISSUES_FILE"' EXIT | |
| ... | |
| CHANGED_FILES="$(git diff --name-only "$BASE_COMMIT..$HEAD_COMMIT" | grep -E '\.(c|h)$' || true)" | |
| ... | |
| local diff_output | |
| diff_output="$(git diff "$BASE_COMMIT..$HEAD_COMMIT" -- "$file")" | |
| ... | |
| if [ -s "$ISSUES_FILE" ]; then | |
| ... | |
| cat "$ISSUES_FILE" | |
| exit 1 | |
| fi |
| output=$(bash .github/scripts/check-pg-versions.sh 2>&1) | ||
| exit_code=$? | ||
| echo "exit_code=${exit_code}" >> $GITHUB_OUTPUT | ||
| echo "output<<EOF" >> $GITHUB_OUTPUT | ||
| echo "$output" >> $GITHUB_OUTPUT | ||
| echo "EOF" >> $GITHUB_OUTPUT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Use a unique delimiter and printf when writing multiline output to $GITHUB_OUTPUT so script output cannot accidentally terminate the block or break formatting. [Learned best practice, importance: 6]
| output=$(bash .github/scripts/check-pg-versions.sh 2>&1) | |
| exit_code=$? | |
| echo "exit_code=${exit_code}" >> $GITHUB_OUTPUT | |
| echo "output<<EOF" >> $GITHUB_OUTPUT | |
| echo "$output" >> $GITHUB_OUTPUT | |
| echo "EOF" >> $GITHUB_OUTPUT | |
| output="$(bash .github/scripts/check-pg-versions.sh 2>&1)" | |
| exit_code=$? | |
| delimiter="EOF_$(date +%s%N)" | |
| { | |
| echo "exit_code=${exit_code}" | |
| echo "output<<${delimiter}" | |
| printf '%s\n' "$output" | |
| echo "${delimiter}" | |
| } >> "$GITHUB_OUTPUT" |
Branch Targeting SuggestionYou've targeted the
If This is an automated suggestion to help route contributions to the appropriate branch. |
PR Type
Enhancement
Description
Add GitHub Action to detect Parameter Group version increment issues
Improve PG version check robustness and efficiency
Add Ray Morris (Sensei) to AUTHORS file
Document GitHub Actions workflows and best practices
Diagram Walkthrough
File Walkthrough
check-pg-versions.sh
Parameter Group version detection script.github/scripts/check-pg-versions.sh
git diffs
were modified
detected
recommendations
error handling
pg-version-check.yml
GitHub Actions workflow for PG version checks.github/workflows/pg-version-check.yml
maintenance-10.x
README.md
GitHub Actions workflows documentation.github/workflows/README.md
AUTHORS
Add Ray Morris to AUTHORSAUTHORS
order