Addressing PR comments#51
Conversation
|
In .github/workflows/check-dependencies.yml regel 69 staat een typo volgensmij. bashecho "current=$CURRENT_OUTPUT" >> $GITHUB_OUTPUT |
- Auto-update workflow now modifies files directly instead of just creating empty PRs - Automatically extracts versions from issues and updates installer scripts - Creates PRs with actual code changes for Ansible, Kubernetes, and NGINX - Marks NGINX PRs as draft (requires SHA256 checksum verification) - Add helper script to calculate and update NGINX SHA256 checksums - Enhanced README documentation explaining true self-maintenance The repository can now maintain itself - detects updates, creates PRs with code changes, only needs human review before merging. Agent-Logs-Url: https://github.com/Stensel8/Scripts/sessions/547a61b6-040d-4e32-ab08-55063bbe8ef5 Co-authored-by: Stensel8 <102481635+Stensel8@users.noreply.github.com>
Added TESTING_AUTOMATION.md with detailed instructions for testing the enhanced automated dependency management system Agent-Logs-Url: https://github.com/Stensel8/Scripts/sessions/547a61b6-040d-4e32-ab08-55063bbe8ef5 Co-authored-by: Stensel8 <102481635+Stensel8@users.noreply.github.com>
…RSION) Fixed typo on line 65 where CURRENT_OUTPUT was used instead of CURRENT_VERSION, which would cause the PCRE2 current version to be empty in the workflow output. Agent-Logs-Url: https://github.com/Stensel8/Scripts/sessions/fd0f5acc-490c-4a4f-b409-1ad4d50bdc39 Co-authored-by: Stensel8 <102481635+Stensel8@users.noreply.github.com>
Fixed in commit 749b8a8. Changed |
There was a problem hiding this comment.
Pull request overview
Updates the repo’s automated dependency management docs and workflows so dependency issues can trigger PRs that include actual version bumps (plus helper tooling for NGINX checksum updates).
Changes:
- Refines README dependency-update section and documents a new NGINX checksum helper script.
- Fixes PCRE2 output handling in the dependency-check workflow.
- Enhances the auto-update workflow to edit installer files, commit, push, and open PRs; adds testing documentation and a checksum update helper script.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Clarifies the end-to-end dependency update flow and adds NGINX checksum helper usage. |
| .github/workflows/check-dependencies.yml | Fixes PCRE2 step output to use the correct variable. |
| .github/workflows/auto-update-dependencies.yml | Implements automated file updates + commit/push/PR creation and improves issue/PR messaging. |
| .github/scripts/update-nginx-checksums.sh | Adds a helper to compute and apply SHA256 updates for NGINX-related tarballs. |
| .github/TESTING_AUTOMATION.md | Adds manual testing steps and expected behavior for the enhanced automation. |
Comments suppressed due to low confidence (1)
.github/workflows/auto-update-dependencies.yml:71
pulls.listonly returns the first page (default 30). If the repo ever has >30 open PRs, the workflow may fail to detect an existing PR linked to the issue and create duplicates. Use pagination (github.paginate) or the search API to reliably find PRs referencing#${issueNumber}.
// Search for any PR that references this issue
const allPRs = await github.rest.pulls.list({
owner: context.repo.owner,
repo: context.repo.repo,
state: 'open'
});
const linkedPR = allPRs.data.find(pr =>
pr.body && pr.body.includes(`#${issueNumber}`)
);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| execSync(`git add ${files.join(' ')}`, { stdio: 'inherit' }); | ||
|
|
||
| This PR addresses the dependency updates identified in issue #${issueNumber}. | ||
| const commitMessage = `chore: update ${updateType} dependencies | ||
|
|
||
| ### Changes Required | ||
| ${Object.entries(updates).map(([comp, vers]) => `- ${comp}: ${vers.current} → ${vers.latest}`).join('\n')} | ||
|
|
||
| The following files need to be updated: | ||
| ${files.map(f => `- [ ] \`${f}\``).join('\n')} | ||
| Automated update from issue #${issueNumber}`; | ||
|
|
||
| ### Update Information | ||
| execSync(`git commit -m "${commitMessage.replace(/"/g, '\\"')}"`, { stdio: 'inherit' }); | ||
| console.log('Committed changes'); |
There was a problem hiding this comment.
This workflow executes git commit via execSync while embedding content derived from the issue body (versions/components) into the command string. Because execSync runs through a shell, crafted issue content containing $()/backticks can lead to command injection with contents: write permissions. Use execFileSync/spawnSync with an argv array (no shell) for all git commands, and avoid interpolating untrusted data into shell strings.
|
|
||
| # Create temp directory | ||
| TEMP_DIR=$(mktemp -d) | ||
| trap "rm -rf $TEMP_DIR" EXIT |
There was a problem hiding this comment.
trap "rm -rf $TEMP_DIR" EXIT should quote the path to avoid issues if the temp directory ever contains unexpected characters. Prefer trap 'rm -rf "${TEMP_DIR}"' EXIT (and keep the rm argument quoted).
| trap "rm -rf $TEMP_DIR" EXIT | |
| trap 'rm -rf "${TEMP_DIR}"' EXIT |
| sed -i "s/\$NGINX_SHA256 = \"[^\"]*\"/\$NGINX_SHA256 = \"$NGINX_SHA256\"/" nginx/nginx_installer.ps1 | ||
| log_success "Updated NGINX SHA256 in nginx_installer.ps1" | ||
| fi | ||
|
|
||
| if [ -n "$OPENSSL_SHA256" ]; then | ||
| sed -i "s/\$OPENSSL_SHA256 = \"[^\"]*\"/\$OPENSSL_SHA256 = \"$OPENSSL_SHA256\"/" nginx/nginx_installer.ps1 | ||
| log_success "Updated OpenSSL SHA256 in nginx_installer.ps1" | ||
| fi | ||
|
|
||
| if [ -n "$PCRE2_SHA256" ]; then | ||
| sed -i "s/\$PCRE2_SHA256 = \"[^\"]*\"/\$PCRE2_SHA256 = \"$PCRE2_SHA256\"/" nginx/nginx_installer.ps1 | ||
| log_success "Updated PCRE2 SHA256 in nginx_installer.ps1" | ||
| fi | ||
|
|
||
| if [ -n "$ZLIB_SHA256" ]; then | ||
| sed -i "s/\$ZLIB_SHA256 = \"[^\"]*\"/\$ZLIB_SHA256 = \"$ZLIB_SHA256\"/" nginx/nginx_installer.ps1 |
There was a problem hiding this comment.
The sed replacements for nginx/nginx_installer.ps1 won't match the current file format (it uses $Script:NGINX_SHA256 = '...' with single quotes), so the helper script likely won't update PowerShell checksums even though it reports success. Update the patterns to target $Script:*_SHA256 assignments and the quoting/spacing used in the PS1 installer.
| sed -i "s/\$NGINX_SHA256 = \"[^\"]*\"/\$NGINX_SHA256 = \"$NGINX_SHA256\"/" nginx/nginx_installer.ps1 | |
| log_success "Updated NGINX SHA256 in nginx_installer.ps1" | |
| fi | |
| if [ -n "$OPENSSL_SHA256" ]; then | |
| sed -i "s/\$OPENSSL_SHA256 = \"[^\"]*\"/\$OPENSSL_SHA256 = \"$OPENSSL_SHA256\"/" nginx/nginx_installer.ps1 | |
| log_success "Updated OpenSSL SHA256 in nginx_installer.ps1" | |
| fi | |
| if [ -n "$PCRE2_SHA256" ]; then | |
| sed -i "s/\$PCRE2_SHA256 = \"[^\"]*\"/\$PCRE2_SHA256 = \"$PCRE2_SHA256\"/" nginx/nginx_installer.ps1 | |
| log_success "Updated PCRE2 SHA256 in nginx_installer.ps1" | |
| fi | |
| if [ -n "$ZLIB_SHA256" ]; then | |
| sed -i "s/\$ZLIB_SHA256 = \"[^\"]*\"/\$ZLIB_SHA256 = \"$ZLIB_SHA256\"/" nginx/nginx_installer.ps1 | |
| sed -i "s/\$Script:NGINX_SHA256[[:space:]]*=[[:space:]]*'[^']*'/\$Script:NGINX_SHA256 = '$NGINX_SHA256'/" nginx/nginx_installer.ps1 | |
| log_success "Updated NGINX SHA256 in nginx_installer.ps1" | |
| fi | |
| if [ -n "$OPENSSL_SHA256" ]; then | |
| sed -i "s/\$Script:OPENSSL_SHA256[[:space:]]*=[[:space:]]*'[^']*'/\$Script:OPENSSL_SHA256 = '$OPENSSL_SHA256'/" nginx/nginx_installer.ps1 | |
| log_success "Updated OpenSSL SHA256 in nginx_installer.ps1" | |
| fi | |
| if [ -n "$PCRE2_SHA256" ]; then | |
| sed -i "s/\$Script:PCRE2_SHA256[[:space:]]*=[[:space:]]*'[^']*'/\$Script:PCRE2_SHA256 = '$PCRE2_SHA256'/" nginx/nginx_installer.ps1 | |
| log_success "Updated PCRE2 SHA256 in nginx_installer.ps1" | |
| fi | |
| if [ -n "$ZLIB_SHA256" ]; then | |
| sed -i "s/\$Script:ZLIB_SHA256[[:space:]]*=[[:space:]]*'[^']*'/\$Script:ZLIB_SHA256 = '$ZLIB_SHA256'/" nginx/nginx_installer.ps1 |
| - Updates `BUILD_PYTHON_VERSION:-3.14.2` to `BUILD_PYTHON_VERSION:-3.14.3` | ||
| - Updates `pip install ansible==13.3.0` to `pip install ansible==13.5.0` | ||
|
|
||
| 3. **Branch Creation**: Creates a new branch like `automated-update/ansible-1743422410` |
There was a problem hiding this comment.
The example branch name uses a 10-digit timestamp (automated-update/ansible-1743422410), but the workflow generates branch names using Date.now() (milliseconds since epoch), which is typically 13 digits. Update the example to match the actual branch format, or change the workflow to use seconds if that’s the intended convention.
| 3. **Branch Creation**: Creates a new branch like `automated-update/ansible-1743422410` | |
| 3. **Branch Creation**: Creates a new branch like `automated-update/ansible-1743422410123` |
| ps1Content = ps1Content.replace(/\$NGINX_VERSION = "([0-9.]+)"/, `$NGINX_VERSION = "${updates['NGINX'].latest}"`); | ||
| shModified = ps1Modified = true; | ||
| console.log(`Updated NGINX version to ${updates['NGINX'].latest}`); | ||
| } | ||
|
|
||
| if (updates['OpenSSL']) { | ||
| shContent = shContent.replace(/OPENSSL_VERSION="([0-9.]+)"/, `OPENSSL_VERSION="${updates['OpenSSL'].latest}"`); | ||
| ps1Content = ps1Content.replace(/\$OPENSSL_VERSION = "([0-9.]+)"/, `$OPENSSL_VERSION = "${updates['OpenSSL'].latest}"`); | ||
| shModified = ps1Modified = true; | ||
| console.log(`Updated OpenSSL version to ${updates['OpenSSL'].latest}`); | ||
| } | ||
|
|
||
| if (updates['PCRE2']) { | ||
| shContent = shContent.replace(/PCRE2_VERSION="([0-9.]+)"/, `PCRE2_VERSION="${updates['PCRE2'].latest}"`); | ||
| ps1Content = ps1Content.replace(/\$PCRE2_VERSION = "([0-9.]+)"/, `$PCRE2_VERSION = "${updates['PCRE2'].latest}"`); | ||
| shModified = ps1Modified = true; | ||
| console.log(`Updated PCRE2 version to ${updates['PCRE2'].latest}`); | ||
| } | ||
|
|
||
| if (updates['Zlib']) { | ||
| shContent = shContent.replace(/ZLIB_VERSION="([0-9.]+)"/, `ZLIB_VERSION="${updates['Zlib'].latest}"`); | ||
| ps1Content = ps1Content.replace(/\$ZLIB_VERSION = "([0-9.]+)"/, `$ZLIB_VERSION = "${updates['Zlib'].latest}"`); |
There was a problem hiding this comment.
The PowerShell version update regexes don't match the actual nginx/nginx_installer.ps1 variable declarations (they are $Script:NGINX_VERSION = '1.29.7', etc.). As written, the workflow won't update the PS1 installer, but will still mark it modified and include it in files, leading to incorrect PR contents/instructions. Update the patterns to target $Script:*_VERSION assignments and handle single quotes.
| ps1Content = ps1Content.replace(/\$NGINX_VERSION = "([0-9.]+)"/, `$NGINX_VERSION = "${updates['NGINX'].latest}"`); | |
| shModified = ps1Modified = true; | |
| console.log(`Updated NGINX version to ${updates['NGINX'].latest}`); | |
| } | |
| if (updates['OpenSSL']) { | |
| shContent = shContent.replace(/OPENSSL_VERSION="([0-9.]+)"/, `OPENSSL_VERSION="${updates['OpenSSL'].latest}"`); | |
| ps1Content = ps1Content.replace(/\$OPENSSL_VERSION = "([0-9.]+)"/, `$OPENSSL_VERSION = "${updates['OpenSSL'].latest}"`); | |
| shModified = ps1Modified = true; | |
| console.log(`Updated OpenSSL version to ${updates['OpenSSL'].latest}`); | |
| } | |
| if (updates['PCRE2']) { | |
| shContent = shContent.replace(/PCRE2_VERSION="([0-9.]+)"/, `PCRE2_VERSION="${updates['PCRE2'].latest}"`); | |
| ps1Content = ps1Content.replace(/\$PCRE2_VERSION = "([0-9.]+)"/, `$PCRE2_VERSION = "${updates['PCRE2'].latest}"`); | |
| shModified = ps1Modified = true; | |
| console.log(`Updated PCRE2 version to ${updates['PCRE2'].latest}`); | |
| } | |
| if (updates['Zlib']) { | |
| shContent = shContent.replace(/ZLIB_VERSION="([0-9.]+)"/, `ZLIB_VERSION="${updates['Zlib'].latest}"`); | |
| ps1Content = ps1Content.replace(/\$ZLIB_VERSION = "([0-9.]+)"/, `$ZLIB_VERSION = "${updates['Zlib'].latest}"`); | |
| ps1Content = ps1Content.replace(/\$Script:NGINX_VERSION\s*=\s*['"]([0-9.]+)['"]/, `$Script:NGINX_VERSION = '${updates['NGINX'].latest}'`); | |
| shModified = ps1Modified = true; | |
| console.log(`Updated NGINX version to ${updates['NGINX'].latest}`); | |
| } | |
| if (updates['OpenSSL']) { | |
| shContent = shContent.replace(/OPENSSL_VERSION="([0-9.]+)"/, `OPENSSL_VERSION="${updates['OpenSSL'].latest}"`); | |
| ps1Content = ps1Content.replace(/\$Script:OPENSSL_VERSION\s*=\s*['"]([0-9.]+)['"]/, `$Script:OPENSSL_VERSION = '${updates['OpenSSL'].latest}'`); | |
| shModified = ps1Modified = true; | |
| console.log(`Updated OpenSSL version to ${updates['OpenSSL'].latest}`); | |
| } | |
| if (updates['PCRE2']) { | |
| shContent = shContent.replace(/PCRE2_VERSION="([0-9.]+)"/, `PCRE2_VERSION="${updates['PCRE2'].latest}"`); | |
| ps1Content = ps1Content.replace(/\$Script:PCRE2_VERSION\s*=\s*['"]([0-9.]+)['"]/, `$Script:PCRE2_VERSION = '${updates['PCRE2'].latest}'`); | |
| shModified = ps1Modified = true; | |
| console.log(`Updated PCRE2 version to ${updates['PCRE2'].latest}`); | |
| } | |
| if (updates['Zlib']) { | |
| shContent = shContent.replace(/ZLIB_VERSION="([0-9.]+)"/, `ZLIB_VERSION="${updates['Zlib'].latest}"`); | |
| ps1Content = ps1Content.replace(/\$Script:ZLIB_VERSION\s*=\s*['"]([0-9.]+)['"]/, `$Script:ZLIB_VERSION = '${updates['Zlib'].latest}'`); |
| // Create a new branch | ||
| const mainBranch = await github.rest.repos.getBranch({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| branch: context.payload.repository.default_branch | ||
| }); |
There was a problem hiding this comment.
mainBranch is fetched via the API but never used (branch creation is done via git checkout -b). This adds unnecessary API calls and can be removed, or alternatively use the fetched default branch info to ensure the local checkout is on the expected base before creating the new branch.
| ${updateType === 'NGINX' ? ` | ||
| **NGINX requires SHA256 checksum updates:** | ||
|
|
||
| 5. Commit and push your changes: | ||
| After reviewing this PR, you'll need to: | ||
| 1. Download the new NGINX tarball and calculate its SHA256: | ||
| \`\`\`bash | ||
| git add ${files.join(' ')} | ||
| git commit -m "Update ${updateType} dependencies" | ||
| git push | ||
| wget https://nginx.org/download/nginx-${updates['NGINX']?.latest}.tar.gz | ||
| sha256sum nginx-${updates['NGINX']?.latest}.tar.gz | ||
| \`\`\` | ||
| 2. Update the SHA256 checksums in both installer files | ||
| 3. Test the installation on a clean system |
There was a problem hiding this comment.
The generated NGINX PR instructions still tell reviewers to manually wget + sha256sum, but this PR also adds ./.github/scripts/update-nginx-checksums.sh and the README now points to it. Consider updating this PR body text to reference the helper script (and keep a single source of truth for the checksum update process).
| const fs = require('fs'); | ||
| const { execSync } = require('child_process'); | ||
|
|
||
| const issueNumber = context.payload.issue?.number || ${{ github.event.inputs.issue_number }}; |
There was a problem hiding this comment.
issueNumber is built using an unquoted workflow expression: context.payload.issue?.number || ${{ github.event.inputs.issue_number }}. On non-workflow_dispatch runs, ${{ github.event.inputs.issue_number }} expands to an empty string, which will produce invalid JS (|| ;) and break the step. Pass the input via env and read process.env, or wrap the expression in quotes and coerce with Number().
| const issueNumber = context.payload.issue?.number || ${{ github.event.inputs.issue_number }}; | |
| const issueNumber = Number(context.payload.issue?.number || '${{ github.event.inputs.issue_number }}'); |
No description provided.