fix: resolve 5 open issues #45
Conversation
|
There was a problem hiding this comment.
Pull request overview
This PR resolves 5 open issues by transitioning the build system from committing versions.json to the git repository to storing it in a GitHub Release, fixing the latest Docker tag logic, and modifying version checking to return only updated versions. The changes also include removal of the now-unnecessary commit script and addition of research documentation for a potential future matrix-based workflow implementation.
Key Changes:
- Moved
versions.jsonfrom git repository to GitHub Release asset named "versions" - Modified
check-bun-node.tsto return only updated Node.js versions instead of all versions - Fixed
latestDocker tag to use the highest Node.js major version fromversions.jsoninstead of hardcoded value - Removed
commit_changes.shscript and related git push workflow steps - Added
research_matrix.mddocumenting analysis of matrix-based workflow approach
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
versions.json |
Removed from repository (now stored in GitHub Release) |
research_matrix.md |
Added documentation analyzing matrix vs single-builder workflow approaches |
commit_changes.sh |
Removed script that committed versions.json to git (no longer needed) |
check-bun-node.ts |
Modified to filter and return only Node.js versions that differ from versions.json |
build_updated.sh |
Fixed latest tag logic to dynamically determine highest Node major version; changed empty version handling to build nothing instead of all versions |
.github/workflows/release.yml |
Added steps to download/upload versions.json from/to GitHub Release; removed git commit/push steps |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
build_updated.sh
Outdated
| if [ -z "$NODE_VERSIONS_TO_BUILD" ]; then | ||
| IFS=',' read -ra NODE_MAJOR_VERSIONS <<<"$NODE_MAJOR_VERSIONS_TO_CHECK" | ||
| log "No Node.js versions to build." | ||
| NODE_VERSIONS=() | ||
| for node_major_version in "${NODE_MAJOR_VERSIONS[@]}"; do | ||
| node_version=$(cat versions.json | jq -r ".nodejs.\"${node_major_version}\".version") | ||
| if [ "$node_version" != "null" ]; then | ||
| NODE_VERSIONS+=("${node_version//v/}") | ||
| fi | ||
| done | ||
| fi | ||
|
|
||
| log "Building Node versions: ${NODE_VERSIONS[*]}" | ||
|
|
||
| # If BUN_VERSIONS_TO_BUILD is empty, but NODE_VERSIONS_TO_BUILD is not, | ||
| # build all versions from versions.json | ||
| # If BUN_VERSIONS_TO_BUILD is empty, but NODE_VERSIONS_TO_BUILD is not, | ||
| # we used to build all versions from versions.json. | ||
| # But for automatic updates, we want to build NOTHING if no Bun versions are updated. | ||
| if [ -z "$BUN_VERSIONS_TO_BUILD" ]; then | ||
| log "No Bun versions to build." | ||
| BUN_VERSIONS=() | ||
| for bun_version in $(cat versions.json | jq -r '.bun | keys[]'); do | ||
| BUN_VERSIONS+=("${bun_version//v/}") | ||
| done | ||
| fi |
There was a problem hiding this comment.
When NODE_VERSIONS and BUN_VERSIONS are both empty arrays, the nested loops (lines 139-172) won't execute, and versions.json will never be updated. This means the workflow will succeed but won't upload an updated versions.json to the GitHub Release.
If the intent is to skip the build when there are no updates, this is correct. However, it could leave the system in an inconsistent state on first run when the Release doesn't exist yet. Consider adding an initialization step or documenting this behavior.
build_updated.sh
Outdated
| # If NODE_VERSIONS_TO_BUILD is empty, but BUN_VERSIONS_TO_BUILD is not, | ||
| # build all versions from versions.json | ||
| # If NODE_VERSIONS_TO_BUILD is empty, but BUN_VERSIONS_TO_BUILD is not, | ||
| # we used to build all versions from versions.json. | ||
| # But for automatic updates, we want to build NOTHING if no Node versions are updated. | ||
| if [ -z "$NODE_VERSIONS_TO_BUILD" ]; then | ||
| IFS=',' read -ra NODE_MAJOR_VERSIONS <<<"$NODE_MAJOR_VERSIONS_TO_CHECK" | ||
| log "No Node.js versions to build." | ||
| NODE_VERSIONS=() | ||
| for node_major_version in "${NODE_MAJOR_VERSIONS[@]}"; do | ||
| node_version=$(cat versions.json | jq -r ".nodejs.\"${node_major_version}\".version") | ||
| if [ "$node_version" != "null" ]; then | ||
| NODE_VERSIONS+=("${node_version//v/}") | ||
| fi | ||
| done | ||
| fi | ||
|
|
||
| log "Building Node versions: ${NODE_VERSIONS[*]}" | ||
|
|
||
| # If BUN_VERSIONS_TO_BUILD is empty, but NODE_VERSIONS_TO_BUILD is not, | ||
| # build all versions from versions.json | ||
| # If BUN_VERSIONS_TO_BUILD is empty, but NODE_VERSIONS_TO_BUILD is not, | ||
| # we used to build all versions from versions.json. | ||
| # But for automatic updates, we want to build NOTHING if no Bun versions are updated. |
There was a problem hiding this comment.
Duplicate comment on lines 44-48 and 56-60. Both comments say "If NODE_VERSIONS_TO_BUILD is empty, but BUN_VERSIONS_TO_BUILD is not, we used to build all versions from versions.json." The first occurrence (lines 44-45) should be kept, and the duplicate (lines 46-48) should be removed.
Similarly, duplicate comments on lines 56-60 where "If BUN_VERSIONS_TO_BUILD is empty, but NODE_VERSIONS_TO_BUILD is not" is repeated.
build_updated.sh
Outdated
| # Only tag "latest" if this is the latest Node.js version and Debian distro | ||
| # We need to check if the current node_major is the latest one in versions.json | ||
| # This is a bit tricky in bash without parsing JSON again, but we can assume the loop order or check against a known latest. | ||
| # Better approach: The caller knows if it's the latest. | ||
| # For now, let's restrict it to the highest known major version we support (e.g. 25) or check if it's the last one in the list? | ||
| # Actually, the issue says "latest tag should be the last tag built". | ||
| # If we build in order, the last one overwrites 'latest'. | ||
| # BUT, if we build multiple versions, we might overwrite 'latest' with an older version if the loop isn't sorted or if we build an old version update. | ||
| # A safer way is to explicitly check if this node version is the "latest" defined in versions.json. |
There was a problem hiding this comment.
[nitpick] Verbose inline comments explaining the logic for determining the latest tag (lines 117-125) could be condensed for better code readability. The key insight—that we need to check against the highest major version in versions.json—could be captured in 1-2 lines rather than 9 lines of commentary.
| # Only tag "latest" if this is the latest Node.js version and Debian distro | |
| # We need to check if the current node_major is the latest one in versions.json | |
| # This is a bit tricky in bash without parsing JSON again, but we can assume the loop order or check against a known latest. | |
| # Better approach: The caller knows if it's the latest. | |
| # For now, let's restrict it to the highest known major version we support (e.g. 25) or check if it's the last one in the list? | |
| # Actually, the issue says "latest tag should be the last tag built". | |
| # If we build in order, the last one overwrites 'latest'. | |
| # BUT, if we build multiple versions, we might overwrite 'latest' with an older version if the loop isn't sorted or if we build an old version update. | |
| # A safer way is to explicitly check if this node version is the "latest" defined in versions.json. | |
| # Tag "latest" only if this is the highest Node.js major version in versions.json and the distro is Debian. |
| - name: Upload versions.json | ||
| run: | | ||
| gh release create versions versions.json --title "Versions State" --notes "Stores the current versions.json state" || \ | ||
| gh release upload versions versions.json --clobber | ||
| env: | ||
| BUN_VERSIONS_TO_BUILD: ${{ env.BUN_VERSIONS_TO_BUILD }} | ||
| NODE_VERSIONS_TO_BUILD: ${{ env.NODE_VERSIONS_TO_BUILD }} | ||
| DISTROS: ${{ env.DISTROS }} | ||
| - name: Pull changes | ||
| run: git pull -r | ||
| - name: Push changes | ||
| uses: ad-m/github-push-action@77c5b412c50b723d2a4fbc6d71fb5723bcd439aa | ||
| with: | ||
| github_token: ${{ secrets.GITHUB_TOKEN }} | ||
| GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} |
There was a problem hiding this comment.
When both NODE_VERSIONS_TO_BUILD and BUN_VERSIONS_TO_BUILD are empty (no updates detected), the build loop doesn't execute and versions.json is never modified. The "Upload versions.json" step will still run and attempt to create/update the Release with the unmodified (potentially empty) versions.json file from the download step.
This could lead to data loss if the Release doesn't exist yet (file would be {}). Consider adding a condition to skip the upload when no builds occurred, or ensure the file has valid structure before uploading.
check-bun-node.ts
Outdated
| .filter((release) => [20, 22, 24, 25].includes(release?.major || 0)) | ||
| .filter((release) => { | ||
| if (!release) return false; | ||
| const currentVersion = versionsJson.nodejs[release.major]?.version; |
There was a problem hiding this comment.
The code attempts to read versions.json from the file system, but if the GitHub Release download fails (e.g., first run, or release doesn't exist), an empty JSON object {} is created. This will cause a runtime error when trying to access versionsJson.nodejs[release.major] since nodejs property won't exist.
Consider adding a default structure or checking if the property exists:
const versionsJson = await Bun.file("versions.json").json();
const nodejs = versionsJson.nodejs || {};
const currentVersion = nodejs[release.major]?.version;| const currentVersion = versionsJson.nodejs[release.major]?.version; | |
| const nodejs = versionsJson.nodejs || {}; | |
| const currentVersion = nodejs[release.major]?.version; |
build_updated.sh
Outdated
| # BUT, if we build multiple versions, we might overwrite 'latest' with an older version if the loop isn't sorted or if we build an old version update. | ||
| # A safer way is to explicitly check if this node version is the "latest" defined in versions.json. | ||
|
|
||
| local latest_node_major=$(echo "${json_data}" | jq -r '.nodejs | keys | map(tonumber) | max') |
There was a problem hiding this comment.
The latest tag logic depends on versions.json existing and having the correct structure to determine latest_node_major. However, on first run or when the GitHub Release doesn't exist, the downloaded file will be {}, causing jq to return null for the max calculation.
This will cause the comparison "$node_major" == "$latest_node_major" to potentially fail or behave unexpectedly. Consider handling the case when versions.json is empty or malformed:
local latest_node_major=$(echo "${json_data}" | jq -r '.nodejs | keys | map(tonumber) | max // 25')This provides a fallback value of 25 if the calculation fails.
| local latest_node_major=$(echo "${json_data}" | jq -r '.nodejs | keys | map(tonumber) | max') | |
| local latest_node_major=$(echo "${json_data}" | jq -r '.nodejs | keys | map(tonumber) | max // 25') |
| - run: bun install | ||
|
|
||
| - name: Download versions.json | ||
| run: gh release download versions -p versions.json || echo "{}" > versions.json |
There was a problem hiding this comment.
The fallback echo "{}" > versions.json when the release doesn't exist creates an empty JSON object, which will cause multiple downstream issues:
check-bun-node.tswill fail when accessingversionsJson.nodejs[release.major](see related comment)build_updated.shwill fail when trying to extract codename and latest_node_major from the JSON- All versions will be considered "new" on first run
Consider creating a proper initial structure with empty nodejs and bun objects:
gh release download versions -p versions.json || echo '{"bun":{},"nodejs":{}}' > versions.json| run: gh release download versions -p versions.json || echo "{}" > versions.json | |
| run: gh release download versions -p versions.json || echo '{"bun":{},"nodejs":{}}' > versions.json |
check-bun-node.ts
Outdated
| // If current version is not set, or is different (assuming we only get newer versions from nodevu), it's an update. | ||
| // Actually, nodevu returns the LATEST version for that major. | ||
| // So if versions.json has v20.10.0 and nodevu says v20.11.0, we want to build. | ||
| // If versions.json has v20.11.0 and nodevu says v20.11.0, we don't want to build. |
There was a problem hiding this comment.
[nitpick] The inline comments on lines 214-217 are overly verbose and redundant. The core logic—comparing current version with the latest from nodevu—is clear from the code itself. Consider condensing to:
// Only build if the version has changed
return currentVersion !== release.versionWithPrefix;| // If current version is not set, or is different (assuming we only get newer versions from nodevu), it's an update. | |
| // Actually, nodevu returns the LATEST version for that major. | |
| // So if versions.json has v20.10.0 and nodevu says v20.11.0, we want to build. | |
| // If versions.json has v20.11.0 and nodevu says v20.11.0, we don't want to build. | |
| // Only build if the version has changed |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
check-bun-node.ts
Outdated
| .filter((release) => [20, 22, 24, 25].includes(release?.major || 0)) | ||
| .filter((release) => { | ||
| if (!release) return false; | ||
| const currentVersion = versionsJson.nodejs[release.major]?.version; |
There was a problem hiding this comment.
Bug: Missing optional chaining crashes on empty versions.json
When versions.json is downloaded and the GitHub release doesn't exist, the fallback creates an empty object {}. Accessing versionsJson.nodejs[release.major]?.version throws a TypeError because versionsJson.nodejs is undefined, and the optional chaining only applies to the .version access, not to [release.major]. This crashes on first run or when the release doesn't exist. The fix requires optional chaining on versionsJson.nodejs itself, like versionsJson.nodejs?.[release.major]?.version.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| needs: setup | ||
| runs-on: ubuntu-latest | ||
| if: ${{ fromJson(needs.setup.outputs.matrix).include[0] }} | ||
| strategy: | ||
| fail-fast: false | ||
| matrix: ${{ fromJson(needs.setup.outputs.matrix) }} | ||
| steps: | ||
| - uses: actions/checkout@v5 | ||
|
|
||
| - name: Download versions.json | ||
| run: gh release download versions -p versions.json || echo "{}" > versions.json | ||
| env: | ||
| GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
|
|
||
| - name: Setup Docker Buildx | ||
| uses: docker/setup-buildx-action@v3 | ||
|
|
||
| - name: Login to Docker Hub | ||
| uses: docker/login-action@v3 | ||
| with: | ||
| username: ${{ secrets.DOCKER_USERNAME }} | ||
| password: ${{ secrets.DOCKER_TOKEN }} | ||
|
|
||
| - uses: nick-fields/retry@ce71cc2ab81d554ebbe88c79ab5975992d79ba08 | ||
| name: Build and push Docker images | ||
| with: | ||
| timeout_minutes: 120 | ||
| max_attempts: 3 | ||
| retry_on: error | ||
| command: ./build_updated.sh | ||
| command: ./build_single.sh --bun "${{ matrix.bun_version }}" --node "${{ matrix.node_version }}" --distro "${{ matrix.distro }}" | ||
| env: | ||
| REGISTRY: ${{ env.REGISTRY }} | ||
| PLATFORMS: ${{ env.PLATFORMS }} | ||
| NODE_VERSIONS_TO_BUILD: ${{ env.NODE_VERSIONS_TO_BUILD || inputs.nodejs-version }} | ||
| BUN_VERSIONS_TO_BUILD: ${{ env.BUN_VERSIONS_TO_BUILD || inputs.bun-versions }} | ||
| DISTROS: ${{ env.DISTROS || inputs.distros }} | ||
|
|
||
| - name: Commit changes | ||
| run: ./commit_changes.sh | ||
| - name: Upload success artifact | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: build-success-${{ matrix.bun_version }}-${{ matrix.node_version }}-${{ matrix.distro }} | ||
| path: build_success.json | ||
|
|
||
| update-release: | ||
| needs: build | ||
| runs-on: ubuntu-latest | ||
| if: always() && needs.build.result == 'success' | ||
| steps: | ||
| - uses: actions/checkout@v5 | ||
| - uses: oven-sh/setup-bun@v1 | ||
|
|
||
| - name: Download versions.json | ||
| run: gh release download versions -p versions.json || echo "{}" > versions.json |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 days ago
To fix this problem, add an explicit permissions block to the workflow configuration. Assign the minimal permissions required for the workflow/features used in each job. The workflow performs a variety of operations:
- Uses
ghCLI insetup,update-release(e.g. downloading/creating/uploading releases, which requirecontentswrite), - Commits and pushes in
setup(requirescontents: write), - Uses
actions/checkout, uploads and downloads artifacts, but does not write to the repository inbuildorrerun-failed-jobs.
Therefore, set contents: write for the jobs that use gh release commands and/or push to the repo, and set contents: read for jobs that only require read access (like build and rerun-failed-jobs).
For clarity and security, add a permissions: block to each of the four jobs:
setup:contents: writebuild:contents: readupdate-release:contents: writererun-failed-jobs:contents: read
Perform these additions at the top of each job definition in the file.
| @@ -37,6 +37,8 @@ | ||
| # test-job: | ||
| # uses: ./.github/workflows/ci.yml | ||
| setup: | ||
| permissions: | ||
| contents: write | ||
| runs-on: ubuntu-latest | ||
| outputs: | ||
| matrix: ${{ steps.set-matrix.outputs.matrix }} | ||
| @@ -74,6 +76,8 @@ | ||
|
|
||
| build: | ||
| needs: setup | ||
| permissions: | ||
| contents: read | ||
| runs-on: ubuntu-latest | ||
| if: ${{ fromJson(needs.setup.outputs.matrix).include[0] }} | ||
| strategy: | ||
| @@ -115,6 +119,8 @@ | ||
|
|
||
| update-release: | ||
| needs: build | ||
| permissions: | ||
| contents: write | ||
| runs-on: ubuntu-latest | ||
| if: always() && needs.build.result == 'success' | ||
| steps: | ||
| @@ -158,6 +164,8 @@ | ||
| GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
|
|
||
| rerun-failed-jobs: | ||
| permissions: | ||
| contents: read | ||
| runs-on: ubuntu-latest | ||
| needs: [build] | ||
| if: failure() |
| - name: Checkout code | ||
| uses: actions/checkout@v5 | ||
| - uses: actions/checkout@v5 | ||
| - uses: oven-sh/setup-bun@v1 |
There was a problem hiding this comment.
Bug: Missing dependency installation in setup job
The setup job removed the bun install step that was present in the old workflow. The check-bun-node.ts script imports @nodevu/core from package.json dependencies, which won't be available without installing dependencies first. This causes the script to fail when running --cleanup or --matrix commands in subsequent steps.
| env: | ||
| BUN_TAGS_TO_CHECK: ${{ env.BUN_TAGS_TO_CHECK }} | ||
| DISTROS: ${{ env.DISTROS }} | ||
| NODE_MAJOR_VERSIONS_TO_CHECK: ${{ env.NODE_MAJOR_VERSIONS_TO_CHECK }} |
There was a problem hiding this comment.
Bug: Workflow manual trigger inputs completely ignored
The workflow defines manual trigger inputs for bun-versions, nodejs-version, and distros, but these inputs are never referenced in the new implementation. The generateMatrix function only reads from environment variables, not workflow inputs. This breaks manual workflow dispatch functionality where users expect to build specific versions by providing inputs, as was possible in the previous version that used ${{ env.NODE_VERSIONS_TO_BUILD || inputs.nodejs-version }}.
| git add . | ||
| git commit -m "chore: cleanup unsupported versions" | ||
| git push | ||
| fi |
There was a problem hiding this comment.
Bug: Missing git pull creates race condition risk
The cleanup step commits and pushes changes without pulling first. The previous workflow included a git pull -r step before pushing. If multiple workflow runs execute concurrently (e.g., scheduled run and manual trigger), the push operation will fail when the remote has been updated by another run, causing the cleanup step to error.
| fi | ||
|
|
||
| # Create a partial JSON update | ||
| echo "{\"nodejs\": {\"$node_major\": {\"version\": \"v$NODE_VERSION\"}}, \"bun\": {\"$bun_tag\": \"v$BUN_VERSION\"}}" > build_success.json |
There was a problem hiding this comment.
Bug: Codename missing from build success JSON output
The build_success.json output only includes the version field for Node.js entries but omits the name field containing the codename (like "iron" or "jod"). When a new Node major version is built for the first time, the merged versions.json won't have the codename, causing tags like latest-iron-debian to be skipped during subsequent builds since generate_tags checks for non-null codenames before creating those tags.
|
|
||
| if [[ $is_canary == false && "$node_major" == "$latest_node_major" && $distro == "debian" ]]; then | ||
| echo "$REGISTRY/bun-node:latest" | ||
| fi |
There was a problem hiding this comment.
Bug: Empty versions.json causes jq error for latest tag
When versions.json is empty (first run scenario), the jq command .nodejs | keys | map(tonumber) | max fails because .nodejs returns null on {}. This sets latest_node_major to empty, preventing the latest tag from being generated. The old implementation hardcoded the comparison to "20", avoiding this dependency on versions.json contents, while the new dynamic approach breaks on empty input.
| log "Building and Tagging Alpine image with Git" | ||
| retry docker buildx build --sbom=true --provenance=true --platform "$PLATFORMS" -t "$image_name-git" -t "$tag-git" "./src/git/${node_major}/${DISTRO}" --push --provenance=mode=max | ||
| fi | ||
| done |
There was a problem hiding this comment.
Bug: Docker image rebuilt for every tag
The loop rebuilds the same Docker image from scratch for each tag instead of building once with all tags. With potentially 10+ tags per image, this causes the same image to be built 10+ times, significantly increasing build time and resource usage. The docker buildx build command accepts multiple -t flags, so all tags should be collected and passed in a single build command outside the loop.
| run: gh release download versions -p versions.json || echo "{}" > versions.json | ||
| env: | ||
| BUN_VERSIONS_TO_BUILD: ${{ env.BUN_VERSIONS_TO_BUILD }} | ||
| NODE_VERSIONS_TO_BUILD: ${{ env.NODE_VERSIONS_TO_BUILD }} |
There was a problem hiding this comment.
Bug: Matrix job success check prevents partial updates
The update-release job checks needs.build.result == 'success', but since build is a matrix job, this requires ALL matrix jobs to succeed. If any single build fails (e.g., one distro out of 12 combinations), the entire result is failure, preventing the update-release job from running. This means successful builds won't update versions.json in the release, losing track of which versions were actually built.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 14 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fi | ||
|
|
||
| # Create a partial JSON update | ||
| echo "{\"nodejs\": {\"$node_major\": {\"version\": \"v$NODE_VERSION\"}}, \"bun\": {\"$bun_tag\": \"v$BUN_VERSION\"}}" > build_success.json |
There was a problem hiding this comment.
The build_success.json output is missing the Node.js codename field, which is present in the original versions.json structure. This will cause the merged versions.json to lose codename information (e.g., "iron", "jod", "krypton").
The output should include the codename:
codename=$(echo "${json_data}" | jq -r ".nodejs.\"${node_major}\".name")
if [ "$codename" != "null" ] && [ -n "$codename" ]; then
echo "{\"nodejs\": {\"$node_major\": {\"name\": \"$codename\", \"version\": \"v$NODE_VERSION\"}}, \"bun\": {\"$bun_tag\": \"v$BUN_VERSION\"}}" > build_success.json
else
echo "{\"nodejs\": {\"$node_major\": {\"version\": \"v$NODE_VERSION\"}}, \"bun\": {\"$bun_tag\": \"v$BUN_VERSION\"}}" > build_success.json
fi| echo "{\"nodejs\": {\"$node_major\": {\"version\": \"v$NODE_VERSION\"}}, \"bun\": {\"$bun_tag\": \"v$BUN_VERSION\"}}" > build_success.json | |
| # Extract codename from versions.json if available | |
| if [ -f versions.json ]; then | |
| json_data=$(cat versions.json) | |
| codename=$(echo "${json_data}" | jq -r ".nodejs.\"${node_major}\".name") | |
| else | |
| codename="" | |
| fi | |
| if [ "$codename" != "null" ] && [ -n "$codename" ]; then | |
| echo "{\"nodejs\": {\"$node_major\": {\"name\": \"$codename\", \"version\": \"v$NODE_VERSION\"}}, \"bun\": {\"$bun_tag\": \"v$BUN_VERSION\"}}" > build_success.json | |
| else | |
| echo "{\"nodejs\": {\"$node_major\": {\"version\": \"v$NODE_VERSION\"}}, \"bun\": {\"$bun_tag\": \"v$BUN_VERSION\"}}" > build_success.json | |
| fi |
| COPY --from=build /usr/local/bin/bun /usr/local/bin/ | ||
| COPY docker-entrypoint.sh /usr/local/bin/ | ||
| RUN mkdir -p /usr/local/bun-node-fallback-bin && ln -s /usr/local/bin/bun /usr/local/bun-node-fallback-bin/node | ||
| ENV PATH "${PATH}:/usr/local/bun-node-fallback-bin" |
There was a problem hiding this comment.
[nitpick] Adding bun as a symlink to node in the PATH could cause confusion and unexpected behavior. When users run node expecting the real Node.js binary, they'll get Bun instead, which may not be fully compatible with Node.js in all scenarios.
This change creates a potential issue for applications that:
- Use
process.execPathto get the Node.js binary path - Check Node.js version using
node --version - Rely on Node.js-specific features not available in Bun
Consider documenting this behavior clearly or providing an environment variable to disable this fallback.
| ENV PATH "${PATH}:/usr/local/bun-node-fallback-bin" | |
| # Optionally add Bun-as-Node fallback to PATH if BUN_NODE_SYMLINK_FALLBACK=1 | |
| ARG BUN_NODE_SYMLINK_FALLBACK=0 | |
| ENV BUN_NODE_SYMLINK_FALLBACK=${BUN_NODE_SYMLINK_FALLBACK} | |
| ENV PATH="${PATH}${BUN_NODE_SYMLINK_FALLBACK:+:/usr/local/bun-node-fallback-bin}" |
| for file in updates/*.json; do | ||
| if [ -f "$file" ]; then | ||
| # Merge nodejs versions | ||
| # We need to be careful not to overwrite the whole object if we only have partial updates. | ||
| # jq's * operator merges objects recursively. | ||
| jq -s '.[0] * .[1]' versions.json "$file" > versions.json.tmp && mv versions.json.tmp versions.json | ||
| fi | ||
| done |
There was a problem hiding this comment.
The merge logic in the Update versions.json step doesn't handle the case where no build artifacts exist (e.g., if all builds failed or the matrix was empty). The glob pattern updates/*.json will fail to match any files, causing the loop to execute once with file set to the literal string "updates/*.json".
Add a check to handle this case:
shopt -s nullglob # Prevent glob from matching itself if no files found
for file in updates/*.json; do
if [ -f "$file" ]; then
jq -s '.[0] * .[1]' versions.json "$file" > versions.json.tmp && mv versions.json.tmp versions.json
fi
done
# Check if any updates were applied
if [ ! -f versions.json.tmp ]; then
echo "No build artifacts found, skipping versions.json update"
fi| run: | | ||
| gh release create versions versions.json --title "Versions State" --notes "Stores the current versions.json state" || \ | ||
| gh release upload versions versions.json --clobber |
There was a problem hiding this comment.
The gh release create command will fail if a release named "versions" already exists (which it will after the first run). The fallback with || will then try to upload, but the error from the first command might cause confusion in logs.
Additionally, using a release tag named "versions" is unconventional and might conflict with actual version tags in the future. Consider using a more specific tag like "state" or "metadata".
Improve the logic:
# Check if release exists first
if gh release view versions >/dev/null 2>&1; then
gh release upload versions versions.json --clobber
else
gh release create versions versions.json --title "Versions State" --notes "Stores the current versions.json state"
fi| for tag in "${tags[@]}"; do | ||
| log "Tagging $image_name as $tag" | ||
| retry docker buildx build --sbom=true --provenance=true --platform "$PLATFORMS" -t "$image_name" -t "$tag" "./src/base/${node_major}/${DISTRO}" --push --provenance=mode=max | ||
|
|
||
| if [ "$DISTRO" == "alpine" ]; then | ||
| log "Building and Tagging Alpine image with Git" | ||
| retry docker buildx build --sbom=true --provenance=true --platform "$PLATFORMS" -t "$image_name-git" -t "$tag-git" "./src/git/${node_major}/${DISTRO}" --push --provenance=mode=max | ||
| fi | ||
| done | ||
|
|
There was a problem hiding this comment.
The docker build command is executed inside a for loop that iterates over all tags, causing the same image to be built multiple times (once for each tag). This is inefficient and wastes CI time.
The build should happen only once with all -t flags passed in a single command. Consider restructuring like this:
# Build all tags in a single command
tag_args=""
for tag in "${tags[@]}"; do
tag_args="$tag_args -t $tag"
done
log "Building and pushing image with tags: ${tags[*]}"
retry docker buildx build --sbom=true --provenance=true --platform "$PLATFORMS" $tag_args "./src/base/${node_major}/${DISTRO}" --push --provenance=mode=max
if [ "$DISTRO" == "alpine" ]; then
log "Building and Tagging Alpine image with Git"
git_tag_args=""
for tag in "${tags[@]}"; do
git_tag_args="$git_tag_args -t $tag-git"
done
retry docker buildx build --sbom=true --provenance=true --platform "$PLATFORMS" $git_tag_args "./src/git/${node_major}/${DISTRO}" --push --provenance=mode=max
fi| for tag in "${tags[@]}"; do | |
| log "Tagging $image_name as $tag" | |
| retry docker buildx build --sbom=true --provenance=true --platform "$PLATFORMS" -t "$image_name" -t "$tag" "./src/base/${node_major}/${DISTRO}" --push --provenance=mode=max | |
| if [ "$DISTRO" == "alpine" ]; then | |
| log "Building and Tagging Alpine image with Git" | |
| retry docker buildx build --sbom=true --provenance=true --platform "$PLATFORMS" -t "$image_name-git" -t "$tag-git" "./src/git/${node_major}/${DISTRO}" --push --provenance=mode=max | |
| fi | |
| done | |
| # Build all tags in a single command | |
| tag_args="-t \"$image_name\"" | |
| for tag in "${tags[@]}"; do | |
| tag_args="$tag_args -t \"$tag\"" | |
| done | |
| log "Building and pushing image with tags: ${tags[*]}" | |
| retry docker buildx build --sbom=true --provenance=true --platform "$PLATFORMS" $tag_args "./src/base/${node_major}/${DISTRO}" --push --provenance=mode=max | |
| if [ "$DISTRO" == "alpine" ]; then | |
| log "Building and Tagging Alpine image with Git" | |
| git_tag_args="-t \"$image_name-git\"" | |
| for tag in "${tags[@]}"; do | |
| git_tag_args="$git_tag_args -t \"$tag-git\"" | |
| done | |
| retry docker buildx build --sbom=true --provenance=true --platform "$PLATFORMS" $git_tag_args "./src/git/${node_major}/${DISTRO}" --push --provenance=mode=max | |
| fi |
| @@ -1,4 +1,4 @@ | |||
| FROM debian:bullseye-slim AS build | |||
| FROM debian:bookworm-slim AS build | |||
There was a problem hiding this comment.
[nitpick] The Debian base image has been updated from debian:bullseye-slim to debian:bookworm-slim, and the Node.js image from node:25-bullseye to node:25-bookworm. However, Node.js 25 was released in October 2024, and official Node.js Docker images for version 25 only support Debian bookworm (not bullseye), so this change is actually necessary and correct rather than just an upgrade.
While this is a good change, it would be helpful to add a comment in the Dockerfile explaining why bookworm is used for newer Node.js versions, as Debian bookworm (12) is the current stable release.
| } | ||
|
|
||
| /** | ||
| * This will detect, wether --bun or --node is requested |
There was a problem hiding this comment.
Spelling error in the comment: "wether" should be "whether".
| * This will detect, wether --bun or --node is requested | |
| * This will detect, whether --bun or --node is requested |
| build: | ||
| needs: setup | ||
| runs-on: ubuntu-latest | ||
| if: ${{ fromJson(needs.setup.outputs.matrix).include[0] }} |
There was a problem hiding this comment.
The condition if: ${{ fromJson(needs.setup.outputs.matrix).include[0] }} will fail if the matrix JSON is malformed or if accessing .include[0] on an empty array. This can cause the workflow to fail with a cryptic error instead of gracefully skipping the build job.
Use a more robust condition:
if: ${{ needs.setup.outputs.matrix != '' && needs.setup.outputs.matrix != '{"include":[]}' }}Or better yet, add a dedicated output flag in the setup job to indicate whether builds are needed.
| if: ${{ fromJson(needs.setup.outputs.matrix).include[0] }} | |
| if: ${{ needs.setup.outputs.matrix != '' && needs.setup.outputs.matrix != '{"include":[]}' }} |
| git config --local user.name "github-actions[bot]" | ||
| git add . | ||
| git commit -m "chore: cleanup unsupported versions" | ||
| git push |
There was a problem hiding this comment.
[nitpick] The cleanup step commits and pushes directly to the main branch without creating a pull request or obtaining approval. This automated cleanup could accidentally delete version folders that are still in use if the Node.js version detection has issues or if the external API returns incorrect data.
Consider one of the following safer approaches:
- Create a pull request for the cleanup changes instead of direct push
- Add a dry-run mode that logs what would be deleted without actually deleting
- Require manual workflow dispatch approval for cleanup operations
- Add better validation that the versions being deleted are truly EOL
| git push | |
| # Instead of pushing directly, create a pull request for the cleanup changes | |
| gh pr create --title "chore: cleanup unsupported versions" --body "Automated cleanup of unsupported versions." --base main --head ${{ github.ref_name }} |
| const verA = parseFloat(a.split("alpine")[1] || "0"); | ||
| const verB = parseFloat(b.split("alpine")[1] || "0"); | ||
| return verB - verA; |
There was a problem hiding this comment.
The Alpine version detection uses parseFloat() to compare version numbers, which doesn't work correctly for versions like "3.20" vs "3.3". For example, parseFloat("3.3") equals 3.3 and parseFloat("3.20") equals 3.2, making 3.3 > 3.2, which is incorrect.
Use proper version comparison:
tags.sort((a: string, b: string) => {
const verA = a.split("alpine")[1] || "0";
const verB = b.split("alpine")[1] || "0";
// Compare as strings with proper version comparison
const [majorA, minorA] = verA.split('.').map(Number);
const [majorB, minorB] = verB.split('.').map(Number);
if (majorA !== majorB) return majorB - majorA;
return minorB - minorA;
});| const verA = parseFloat(a.split("alpine")[1] || "0"); | |
| const verB = parseFloat(b.split("alpine")[1] || "0"); | |
| return verB - verA; | |
| const verA = a.split("alpine")[1] || "0"; | |
| const verB = b.split("alpine")[1] || "0"; | |
| const [majorA, minorA] = verA.split('.').map(Number); | |
| const [majorB, minorB] = verB.split('.').map(Number); | |
| if (majorA !== majorB) return majorB - majorA; | |
| return minorB - minorA; |
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| DISTROS: ${{ env.DISTROS }} | ||
| NODE_MAJOR_VERSIONS_TO_CHECK: ${{ env.NODE_MAJOR_VERSIONS_TO_CHECK }} |
There was a problem hiding this comment.
The workflow defines workflow_dispatch inputs for bun-versions, nodejs-version, and distros (lines 8-19), but these inputs are not actually used in the new matrix-based workflow. The setup job generates the matrix dynamically from environment variables and doesn't consider the manual inputs.
This means manual workflow triggers with custom versions will be ignored. If this is intentional (forcing all builds to go through the automatic detection), the inputs should be removed from the workflow. Otherwise, the matrix generation logic needs to incorporate these inputs when provided.
| DISTROS: ${{ env.DISTROS }} | |
| NODE_MAJOR_VERSIONS_TO_CHECK: ${{ env.NODE_MAJOR_VERSIONS_TO_CHECK }} | |
| DISTROS: ${{ github.event.inputs.distros != '' && github.event.inputs.distros || env.DISTROS }} | |
| NODE_MAJOR_VERSIONS_TO_CHECK: ${{ env.NODE_MAJOR_VERSIONS_TO_CHECK }} | |
| NODE_VERSIONS_TO_BUILD: ${{ github.event.inputs.nodejs-version != '' && github.event.inputs.nodejs-version || env.NODE_VERSIONS_TO_BUILD }} | |
| BUN_VERSIONS_TO_BUILD: ${{ github.event.inputs['bun-versions'] != '' && github.event.inputs['bun-versions'] || env.BUN_VERSIONS_TO_BUILD }} |
| - name: Cleanup unsupported versions | ||
| run: | | ||
| set -e | ||
| echo "NODE_VERSIONS_TO_BUILD=$(bun run check-bun-node.ts --node ${{ env.NODE_MAJOR_VERSIONS_TO_CHECK }})" >> $GITHUB_ENV | ||
| echo "BUN_VERSIONS_TO_BUILD=$(bun run check-bun-node.ts --bun ${{ env.BUN_TAGS_TO_CHECK }})" >> $GITHUB_ENV | ||
| bun run check-bun-node.ts --cleanup --generate | ||
| if [[ `git status --porcelain` ]]; then | ||
| git config --local user.email "github-actions[bot]@users.noreply.github.com" | ||
| git config --local user.name "github-actions[bot]" | ||
| git add . | ||
| git commit -m "chore: cleanup unsupported versions" | ||
| git push | ||
| fi |
There was a problem hiding this comment.
The cleanup step commits and pushes changes to the repository during the workflow. This can cause race conditions if multiple workflow runs happen simultaneously (e.g., scheduled and manual triggers). Additionally, pushing commits from within a workflow can trigger new workflow runs, potentially causing an infinite loop if not properly guarded.
Consider either:
- Using a separate workflow run for cleanup that runs less frequently
- Using branch protection rules or checking if changes were actually made to unsupported versions
- Using a lock mechanism to prevent concurrent cleanup operations
| if (process.argv.includes("--node")) { | ||
| console.log( | ||
| (await generateReleaseData()) | ||
| // ... (keep existing) | ||
| const releases = await generateReleaseData(); | ||
| const versionsJson = await Bun.file("versions.json").json(); | ||
|
|
||
| const newVersions = releases | ||
| .filter((release) => [20, 22, 24, 25].includes(release?.major || 0)) | ||
| .filter((release) => { | ||
| if (!release) return false; | ||
| const currentVersion = versionsJson.nodejs[release.major]?.version; | ||
| // If current version is not set, or is different (assuming we only get newer versions from nodevu), it's an update. | ||
| // Actually, nodevu returns the LATEST version for that major. | ||
| // So if versions.json has v20.10.0 and nodevu says v20.11.0, we want to build. | ||
| // If versions.json has v20.11.0 and nodevu says v20.11.0, we don't want to build. | ||
| return currentVersion !== release.versionWithPrefix; | ||
| }) | ||
| .map((release) => release?.versionWithPrefix.replace("v", "")) | ||
| .join(",") | ||
| ); | ||
| .join(","); | ||
|
|
||
| console.log(newVersions); | ||
| } |
There was a problem hiding this comment.
The --node command will fail if versions.json doesn't exist. The code attempts to read versions.json at line 787 without error handling, while the --matrix command has proper try-catch handling for this case (lines 667-671).
This inconsistency means the --node command cannot work on first run or when versions.json is missing, even though it should be able to output all current versions.
Suggested fix:
if (process.argv.includes("--node")) {
const releases = await generateReleaseData();
let versionsJson: any = {};
try {
versionsJson = await Bun.file("versions.json").json();
} catch {
// If file doesn't exist, treat all versions as new
}
const newVersions = releases
.filter((release) => [20, 22, 24, 25].includes(release?.major || 0))
.filter((release) => {
if (!release) return false;
const currentVersion = versionsJson.nodejs?.[release.major]?.version;
return currentVersion !== release.versionWithPrefix;
})
.map((release) => release?.versionWithPrefix.replace("v", ""))
.join(",");
console.log(newVersions);
}| for await (const folder of glob.scan({ cwd: absoluteDir, absolute: false, onlyFiles: false })) { | ||
| if (!supportedMajors.includes(folder)) { | ||
| const folderPath = join(absoluteDir, folder); | ||
| try { | ||
| const stats = await (await import("node:fs/promises")).stat(folderPath); | ||
| if (stats.isDirectory()) { | ||
| await rm(folderPath, { recursive: true, force: true }); | ||
| } | ||
| } catch (e) { | ||
| // Ignore errors (e.g., file does not exist) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The cleanup logic only checks if a folder name is a supported major version, but doesn't validate if the folder is actually a version folder. This could accidentally delete non-version directories in src/base or src/git (e.g., if someone creates a documentation folder, shared scripts, etc.).
Consider adding validation to ensure the folder name is numeric before attempting deletion:
for await (const folder of glob.scan({ cwd: absoluteDir, absolute: false, onlyFiles: false })) {
// Only process numeric folder names (version folders)
if (!/^\d+$/.test(folder)) continue;
if (!supportedMajors.includes(folder)) {
const folderPath = join(absoluteDir, folder);
// ... rest of deletion logic
}
}| max_attempts: 3 | ||
| retry_on: error | ||
| command: ./build_updated.sh | ||
| command: ./build_single.sh --bun "${{ matrix.bun_version }}" --node "${{ matrix.node_version }}" --distro "${{ matrix.distro }}" |
There was a problem hiding this comment.
Missing executable permission. The script is being called directly with ./build_single.sh but there's no step to make it executable. While this might work if the file is already committed with executable permissions, it's safer to ensure it explicitly in the workflow.
Add before the build step:
- name: Make build script executable
run: chmod +x build_single.sh| COPY docker-entrypoint.sh /usr/local/bin | ||
| COPY --from=build /usr/local/bin/bun /usr/local/bin/bun | ||
| RUN mkdir -p /usr/local/bun-node-fallback-bin && ln -s /usr/local/bin/bun /usr/local/bun-node-fallback-bin/node |
There was a problem hiding this comment.
[nitpick] Creating a symlink from /usr/local/bin/bun to /usr/local/bun-node-fallback-bin/node creates a potential conflict. This symlink effectively makes node point to bun, which could cause issues with tools that explicitly expect the Node.js binary behavior. While Bun has Node.js compatibility, there are subtle differences that might break tools.
Consider:
- Adding a comment explaining why this fallback is needed and what use cases it supports
- Documenting potential compatibility issues
- Making this optional via a build argument so users can disable it if needed
Example:
# Optional: Create bun-as-node fallback for tools that hard-code 'node' binary
# Note: Bun has high Node.js compatibility but some differences exist
ARG ENABLE_NODE_FALLBACK=true
RUN if [ "$ENABLE_NODE_FALLBACK" = "true" ]; then \
mkdir -p /usr/local/bun-node-fallback-bin && \
ln -s /usr/local/bin/bun /usr/local/bun-node-fallback-bin/node; \
fi
ENV PATH "${PATH}:/usr/local/bun-node-fallback-bin"| RUN mkdir -p /usr/local/bun-node-fallback-bin && ln -s /usr/local/bin/bun /usr/local/bun-node-fallback-bin/node | |
| # Optional: Create bun-as-node fallback for tools that hard-code 'node' binary | |
| # Note: Bun has high Node.js compatibility but some differences exist. | |
| # See https://github.com/oven-sh/bun/issues for known incompatibilities. | |
| ARG ENABLE_NODE_FALLBACK=true | |
| RUN if [ "$ENABLE_NODE_FALLBACK" = "true" ]; then \ | |
| mkdir -p /usr/local/bun-node-fallback-bin && \ | |
| ln -s /usr/local/bin/bun /usr/local/bun-node-fallback-bin/node; \ | |
| fi |
| } | ||
|
|
||
| /** | ||
| * This will detect, wether --bun or --node is requested |
There was a problem hiding this comment.
Typo: "wether" should be "whether".
| * This will detect, wether --bun or --node is requested | |
| * This will detect, whether --bun or --node is requested |
latesttag should be the last tag built and pushed to dockerhub #38Changes
src/baseandsrc/git.commit_changes.shasversions.jsonis no longer committed.latesttag logic inbuild_single.sh.versions.jsonto GitHub Releaseversions.Verification
Note
Switches release to a matrix-based workflow using per-combination builds, auto-generates/cleans Dockerfiles, updates base images, and stores versions.json in a GitHub Release.
setupjob to generate dynamic build matrix fromcheck-bun-node.tsand clean unsupported versions; commits cleanup.buildjob invoking./build_single.shper{bun,node,distro}and uploading success artifacts.update-releasejob to merge artifacts intoversions.jsonand upload to GitHub Releaseversions.build.build_single.shto build/tag/push a single image combo, emit partialbuild_success.json, and refinelatesttag logic.build_updated.shandcommit_changes.sh.versions.json; read/write it from/to a GitHub Release.--cleanup,--generate, and--matrixmodes: cleanup EOL Node folders, generate Dockerfiles, and output matrix based on npm tags andversions.json.3.22and Debian base tobookwormvariants acrosssrc/base/*andsrc/git/*.bun-nodeshim toPATHin Dockerfiles.docs/research_matrix.mdcomparing single builder vs matrix approach.Written by Cursor Bugbot for commit 77977ef. This will update automatically on new commits. Configure here.