Repository navigation
Fix false booleans in config file being parsed as true - #8053
Merged
Merged
Conversation
Options like `enable-remote-storage: false` in config.yaml are converted to `--enable-remote-storage=false`, but the parser set any boolean option to true regardless of its value. Booleans set to false are now ignored, the same way optional strings set to false already are. Fixes coder#8049
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
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.
Fixes #8049
Summary
Boolean options set to
falseinconfig.yamlwere being treated astrue. This PR makesparse()ignore booleans whose value isfalse, the same way it already ignores optional strings set tofalse.Problem
parseConfigFile()turns each config entry into a CLI-style flag (src/node/cli.ts#L815-L824):enable-remote-storage: true--enable-remote-storageenable-remote-storage: false--enable-remote-storage=falseparse()then sets any boolean option it sees totrue, without looking at the value (src/node/cli.ts#L438-L441):So writing
falsein the config file silently does the opposite of what the user asked. This affects every boolean option, for exampledisable-telemetry,disable-update-check,disable-workspace-trustandenable-remote-storage(the case reported in #7914).Solution
Skip the assignment when a boolean's value is
"false":Why this approach
falseare already ignored in the same function (#L460), and there's a test for it (--cert=false→{}).parseConfigFile(), so--flag=falseon the command line now works too instead of enabling the flag.false) keeps the existing merge semantics between the config file and CLI args untouched.Changes
src/node/cli.tsfalse(+5 / −1)test/unit/node/cli.test.tsBefore / After
config.yaml:{ "disable-telemetry": true, "disable-update-check": true }❌{ "disable-update-check": true }✅How to test
enable-remote-storage: false(or any boolean option set tofalse) to~/.config/code-server/config.yaml.code-server --disable-telemetry=falseand confirm telemetry is no longer disabled.Testing done
should ignore booleans set to false:parse(["--disable-telemetry=false"])returns{}.should parse booleans set to false in the config file: a config with onefalseand onetrueboolean only keeps thetrueone.npm run test:unit).tsc --noEmitare clean.Notes for reviewers
some-flag: falsein their config and was (unknowingly) relying on it being enabled will now get it disabled, which is what their config says. I'd consider this a bug fix, but it may be worth a changelog note.