Fix update.sh Dockerfile status reporting#2308
Fix update.sh Dockerfile status reporting#2308PeterDaveHello wants to merge 1 commit intonodejs:mainfrom
Conversation
Compare each generated Dockerfile with its existing copy before replacing it, so the script only announces an update when the content actually changes. Otherwise keep the original file and print an 'already up to date' message to eliminate false positives.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the Dockerfile update logic in update.sh to improve efficiency and consistency. The main goal is to avoid unnecessary file operations and use consistent output messaging.
- Moved the YARN_VERSION sed operation to always execute before comparison (when SKIP is false)
- Changed file comparison from
diff -qtocmp -sand added conditional handling to avoid overwriting unchanged files - Standardized output messages to use the
infofunction instead ofecho
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fi | ||
|
|
||
| mv -f "${dockerfile}-tmp" "${dockerfile}" | ||
| if cmp -s "${dockerfile}-tmp" "${dockerfile}"; then |
There was a problem hiding this comment.
Not too important in terms of functionality but out of curiosity what made you choose cmp over the more common diff here?
There was a problem hiding this comment.
I don't have a preference actually 😅
|
This PR may now be obsolete
This issue was reported in #2413 and fixed in #2416 Also Yarn is now handled differently due to it being removed in the future. |
|
@MikeMcC399 Thanks. I'll close this one now, although I'm not sure why we're creating new PRs instead of using existing ones. Maybe this is just the current state of the project. |
Thank you!
PR #2416 was to remove Yarn, and because there are no more Yarn releases expected, it also simplified the Yarn version handling. A by-product was to fix #2413 regarding reporting the update state (updated or not updated), so from my viewpoint it wasn't submitted to replace your PR, it just happened to fix the issue. @nschonni can comment additionally since he was the author of the PR. I'm not a maintainer, just a contributor, so I can't really comment in general about the organization of the project. There is however now a Slack channel set up for those sorts of discussions. |
|
The new PR isn't intended to replace this one, I believe that. My small concern is the earlier discussion about the maintenance status of this project. This PR was created months ago; some issues could have been fixed earlier. Maybe most of us don't have enough time to review PRs, so we create a new one when needed. |
Description
Clarify
update.shstatus messages by comparing each generated Dockerfile with the existing file before replacing it. Move the Yarn version update ahead of the comparison and emit either “updated” when the content changes or “already up to date.” when it does not.GitHub Copilot Summary
Motivation and Context
Previous runs could print “updated!” even when the final Dockerfile was unchanged, leading to confusing output after
git statusshowed no diffs. This change makes the script’s messaging align with the actual state of the files.Testing Details
Example Output(if appropriate)
Types of changes
Checklist