fix: stop silently discarding a user's config on decode failure - #90
Merged
Conversation
resolvedSystemConfig() used `try?` around ConfigurationLoader.load(), so any decode failure silently fell back to an all-defaults ContainerSystemConfig — not just for the field that failed, the user's entire config.toml (DNS, build settings, kernel, everything). ConfigurationLoader.load() only throws when a config file exists on disk but fails to parse/decode; it already returns cleanly with defaults when no file is present at all. So every error caught here represents real discarded user data, not an absent-file no-op. container 1.2.0 made this concrete: KernelConfig's decoder now throws when kernel.url is customized without a paired kernel.digest (apple/container#1703) — any Berthly user who ran `container system kernel set --tar <url>` pre-1.2.0 has exactly that config shape, and would silently lose their whole system config on next load once the daemon is upgraded. Extracts the load-outcome handling into a pure, testable mapSystemConfigLoadResult(_:) and surfaces a failure through the existing lastStartupWarning mechanism instead of swallowing it.
Uh oh!
There was an error while loading. Please reload this page.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
resolvedSystemConfig()usedtry?aroundConfigurationLoader.load(), so any decode failure silently fell back to an all-defaultsContainerSystemConfig— not just the field that failed, the user's entireconfig.toml(DNS, build settings, kernel, everything).ConfigurationLoader.load()only throws when a config file exists on disk but fails to parse/decode; it already returns cleanly with defaults when no file is present at all — so every error caught here represents real discarded user data, not an absent-file no-op.KernelConfig's decoder now throws whenkernel.urlis customized without a pairedkernel.digest(Verify kernel archive integrity apple/container#1703). Any Berthly user who rancontainer system kernel set --tar <url>pre-1.2.0 has exactly that config shape, and would silently lose their whole system config on next load once the daemon is upgraded.mapSystemConfigLoadResult(_:)and surfaces a failure through the existinglastStartupWarningmechanism instead of swallowing it.Why
Found while implementing #79 (kernel digest verification) — this milestone's own version bump is what triggers the failure for affected users, so it needs fixing in the same milestone.
Closes#87
Test plan
xcodebuild buildsucceedsxcodebuild test -only-testing:BerthlyTests— full suite passes, including newSystemConfigLoadResultMappingTestscovering both the success and failure pathsswiftlint lint --strict— 0 violations