attempt to separate security releases from experimental musl releases#2348
attempt to separate security releases from experimental musl releases#2348bmuenzenmeyer wants to merge 1 commit intonodejs:mainfrom
Conversation
|
How can we get musl build issued when they are available? |
| updatedVersions.push(newVersion.fullVersion); | ||
| } else { | ||
| console.log(`There's no musl build for version ${newVersion.fullVersion} yet.`); | ||
| process.exit(0); |
There was a problem hiding this comment.
AI suggested this should be removed during a self-review. This would prevent the loop from running to another entry, meaning we wait for another run. Maybe this is intentional?
good question. this doesnt address that at all. subsequent runs will skip it unless we introduce more state somehow. open to suggestions from repo owners |
There was a problem hiding this comment.
Pull request overview
This PR modifies the build automation logic to prioritize security releases over waiting for experimental musl builds. Previously, all releases were blocked until musl builds were available. Now, security releases proceed immediately with the -s flag (which skips Alpine and Yarn updates), while non-security releases still wait for musl builds. Additionally, when musl builds are unavailable for non-security releases, the process now continues instead of exiting.
Changes:
- Security releases are now processed independently of musl build availability
- Non-security releases continue to wait for musl builds before processing
- Changed from
process.exit(0)to skipping when musl builds are unavailable for non-security releases
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (newVersion.isSecurityRelease) { | ||
| console.log(`Processing security release ${newVersion.fullVersion}`); | ||
| const { stdout } = await exec(`./update.sh -s ${version}`); | ||
| console.log(stdout); | ||
| updatedVersions.push(newVersion.fullVersion); | ||
| } else if (newVersion.muslBuildExists) { |
There was a problem hiding this comment.
When a security release doesn't have a musl build yet, it will still be processed with the -s flag which skips Alpine updates. However, according to update.sh line 23, the -s flag 'skip[s] updating the yarn and alpine versions.' This means security releases without musl builds will proceed but Alpine variants won't be updated. The original logic required muslBuildExists to be true before processing any release. Consider whether security releases should also check for muslBuildExists, or if this behavior is intentional and should be documented.
| } else { | ||
| console.log(`There's no musl build for version ${newVersion.fullVersion} yet.`); | ||
| process.exit(0); | ||
| console.log(`There's no musl build for version ${newVersion.fullVersion} yet. Skipping non-security release.`); |
There was a problem hiding this comment.
The removal of process.exit(0) changes the behavior significantly. Previously, the absence of a musl build would stop all processing. Now it continues to the next version in the loop. If multiple versions are being processed and one lacks a musl build, this could lead to partial updates. Consider whether this is the intended behavior and if it should be logged more prominently (e.g., as a warning) to ensure visibility of skipped versions.
I think a separate version checking for alpine and non-alpine is always required. Whipped up a simple attempt here: ItsHarta@f819843 A bit repetitive, but I think it should handles skipped builds & more robust checking in general. Happy to open a separate PR if it's good enough |
3 participants
Description
Attempt to separate security releases from experimental musl releases.
Motivation and Context
See attribution of idea at #2330 (comment) by @MikeMcC399
Waiting for musl builds is unnecessary. See impact docker-library/official-images#20637
@mcollina confirmed this may be the source of the bug.
I am no expert in this domain, so special care should be made and I have no hard feelings about suggested improvements.
I did elect for explicit cases here over ternary logic, as I felt it created annoying inline branches in the update arguments and in the console log. Now it's more explicit, at the cost of slight repetition.
Testing Details
Example Output(if appropriate)
Types of changes
Checklist