fix: avoid overwriting malformed global config#1339
Conversation
Hweinstock
left a comment
Hweinstock
left a comment
There was a problem hiding this comment.
Thanks for taking a look! Looking at this change, we might need a broader refactor here to get the right behavior we're looking for.
There was a problem hiding this comment.
it looks like this catch will still silently swallow all errors from downstream. The result is that the user won't see a helpful error message of why the command failed when their config is invalid, but will see that the command failed.
I think this might take a broader refactor to bubble up this error correctly, (and maybe unify these read config methods)
kiwigitops
changed the title
|
That makes sense. This patch handles the overwrite case, but it still leaves callers without a clean way to distinguish a missing config from a malformed config. I can rework this around returning the parse/read error from the config-loading path if that matches the direction you had in mind. |
Return a {success, error} result instead of a bare boolean so callers can distinguish a missing config (start fresh) from a malformed one (left unchanged, with the parse/read error surfaced). Telemetry enable/disable now report the actual failure reason rather than a generic write warning.
|
Reworked along those lines. |
Hweinstock
left a comment
Hweinstock
left a comment
There was a problem hiding this comment.
Nice! The user experience makes sense to me. Few implementation notes, but I like the direction.
| * (treated as an empty config) from a malformed one (read/parse/schema failure), | ||
| * so the caller can avoid clobbering a config it could not understand. | ||
| */ | ||
| async function loadConfigForUpdate(configFile: string): Promise<LoadConfigResult> { |
There was a problem hiding this comment.
this logic feels a bit convoluted to me. Do you think it makes sense to directly check if the file exists (via fs.exists or something), then use that instead of the error?
| } catch (error) { | ||
| return { | ||
| success: false, | ||
| error: `Config at ${configFile} is malformed and was left unchanged: ${toError(error).message}`, |
There was a problem hiding this comment.
nit: i don't think we need the "left unchanged"
| } | ||
| } | ||
|
|
||
| type LoadConfigResult = { success: true; config: GlobalConfig } | { success: false; error: string }; |
There was a problem hiding this comment.
can we call the field errorMsg or make it store the full error object?
What you did is in-line with an existing codebase pattern I'm trying to undo where we maintain errors as strings everywhere. This makes sense for displaying to the user, but creates issues when we want programmatic logic based on errors (ex. check if downstream threw an error of type X). Without the object, we lose error type information and are forced to rely on brittle string matching to determine the cause of the error.
|
|
||
| it('returns false on write failures', async () => { | ||
| const ok = await updateGlobalConfig( | ||
| it('does not overwrite malformed config and reports why', async () => { |
2 participants
Summary
Fixes #1337.
Testing
npx prettier --check src/lib/schemas/io/global-config.ts src/cli/__tests__/global-config.test.tsnpx vitest run --project unit src/cli/__tests__/global-config.test.ts