Quality sweep: build integrity, script robustness and stale defaults - #1134
Merged
Merged
Conversation
Three real bugs, plus the configuration drift that came with them. `init-mosquitto` linked mosquitto_pub and mosquitto_rr with a bare `ln -s`, which fails once the links exist. Bashio runs with errexit, so the second start of a container that has MQTT available failed the oneshot outright. This is the same class of failure #922 fixed for the home folder links and did not cover here. Both links now use `-f` and carry a warning, matching the rest of the script. None of the downloads in the Dockerfile used `--fail`, so an HTTP error produced a file rather than an error. The Home Assistant CLI was the bad case: a wrong version wrote the 404 body to /usr/bin/ha, marked it executable, and the build passed. All four downloads now fail loudly. The cleanup pass never removed a single directory. In `find`, `-a` binds tighter than `-o`, so the expression parsed as "first group, or second group and delete", leaving the `-exec` reachable only through the second branch. The name tests are now grouped so it applies to both. This drops 1450 leftover test, tests and __pycache__ directories from the image. The default settings had drifted since 2022. VS Code removed `terminal.integrated.shell.linux` entirely, so the intended zsh terminal was coming only from the shell recorded in /etc/passwd; it is replaced by the profile settings. `telemetry.enableTelemetry` gives way to `telemetry.telemetryLevel`, and the vsicons entries went, as the icons extension is not installed. The outgoing default is recorded in PREVIOUS_DEFAULT_CONFIG_HASHES so existing installs pick this up. Two of the eight bundled extensions had no Renovate manager, and there was one manager for an extension that is not bundled at all. `redhat.vscode-yaml` gains a manager and moves from a pre-release build onto the stable line it tracks. `oderwat.indent-rainbow` stays manual, as its repository publishes no tags or releases and Renovate has no registry datasource that covers it. The rest is smaller: the documentation and the package install message said Ubuntu while the base is Debian, `init-mysql` created a folder it never wrote to, the readme template carried the old name, the unused workspaces.json is gone, and the hadolint ignores no longer applied to anything, so the linter now enforces pinned apt versions again.
frenck
added
bugfix
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review. WalkthroughThe PR updates VS Code Server image downloads, extension tracking, runtime initialization scripts, editor defaults, and documentation. It also removes the Code Server workspace state file and aligns package references with Debian. ChangesVS Code Server maintenance
Estimated code review effort: 2 (Simple) | ~15 minutes Merge Risk: 🔵 Low · up to The PR now writes database credentials in a system-wide MySQL configuration directory without explicitly defining restrictive access permissions. It is mergeable with owner awareness or follow-up to confirm that only intended processes can read the file. Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (10 skipped: 10 unsupported.) ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed Changes
A full pass over the app: Dockerfile, S6 scripts, defaults, docs and Renovate configuration. Everything below was verified against a local amd64 build rather than read off the page.
Bugs
init-mosquittofailed on any restart of a container with MQTT available. The two convenience links were created with a bareln -s, which fails once they exist. Bashio runs witherrexit, so the oneshot exited 1 and s6 could not bring the bundle up. This is the same class of failure #922 fixed for the home folder links, and it did not reach this script. Both links now use-fand log a warning, matching the rest of the script.Downloads could not fail. No
curlin the Dockerfile used--fail, so an HTTP error wrote the error body to the target path. The Home Assistant CLI was the bad one:A wrong
HA_CLI_VERSIONshipped a 404 page as/usr/bin/ha, executable, and the build passed. All four downloads now use--fail.The cleanup pass never deleted a directory. In
find,-abinds tighter than-o, so the expression parsed asGROUP1 -o (GROUP2 -a -exec)and the-execwas reachable only through the.pycbranch. The-type dalso applied totestalone, so files namedtestsor__pycache__would have matched.test/tests/__pycache__directories.pycfiles/usr/localStale defaults
settings.jsonhad not been touched since 2022-11-09 and had drifted:terminal.integrated.shell.linuxwas removed from VS Code. It appears in 0 files of the bundled 1.135, against 5 forterminal.integrated.defaultProfile.linux. The intended zsh terminal was coming only from the shell recorded in/etc/passwd. Replaced with the profile settings.telemetry.enableTelemetryis the deprecated alias fortelemetry.telemetryLevel.vsicons.*entries are inert, as the icons extension is not bundled.The outgoing default's hash is added to
PREVIOUS_DEFAULT_CONFIG_HASHES, so existing installs receive the change. Verified both directions:Renovate
Two of the eight bundled extensions had no manager and had silently stopped being updated, and there was a manager for
netcorext.uuid-generator, which is not bundled.redhat.vscode-yamlgains a manager. It also moves from1.25.2026082008to1.24.0: that version string is a pre-release build, upstream's newest release is1.24.0, and the pin before it (1.11.10112022) followed the same pattern, so this app has been shipping that extension's pre-release channel for years.1.24.0is on the Marketplace and needs^1.63.0, satisfied by Code 1.135.oderwat.indent-rainbowstays manual and cannot be automated: its repository has no tags and no releases, and Renovate has no Open VSX or Marketplace datasource.8.3.1is current, and upstream has not been pushed to since August 2024.Smaller items
packagesoption as taking Ubuntu packages and linkedpackages.ubuntu.com; the base is Debian. The failure message ininit-usersaid the same. Both corrected, andapt updatebecameapt-get update, which is the interface meant for scripts.init-mysqlcreated/root/.configbut wrote to/etc/mysql/conf.d. Harmless today becausemariadb-clientships that folder, but it was protecting nothing. Verified the script now succeeds with the folder removed beforehand.workspaces.jsonwas referenced by nothing. Removed.extensionandvendorin a single statement, which worked only because bash evaluates assignments left to right, and$?was tested instead of the command. Both straightened out, along with theextentionandConfiuretypos.# hadolint ignore=SC2181, DL3008no longer applied to anything: SC2181 covered the$?test that is gone, and every apt package is pinned. Verified hadolint is clean with no ignores, so the line is removed and the linter enforces pinned versions again.Verification
Full amd64 build, plus Shellcheck, Prettier, YAMLLint, Hadolint and
renovate-config-validator, all clean. Thehabinary in the built image is a real ELF and runs, all eight extensions install at the expected versions, and the S6 scripts were exercised directly for the behaviour described above.Related Issues
Follows the
.sshlink fix in #1098 and the guarded links added in #922.Summary by CodeRabbit
New Features
Bug Fixes
Documentation