fix: update config file handling in application initialization#26
fix: update config file handling in application initialization#26dmisic-godaddy wants to merge 3 commits intogodaddy:mainfrom
Conversation
|
A bit more context on why I think the current fix is too broad: The issue here is not just "missing config file". It is really that Current behavior is roughly:
Given that, I think the intended behavior should be:
The current patch fixes (3), but it also changes (4): on the env-only path it catches every A few concrete implications:
Stepping back, I think the bigger design fix is:
If we do keep a command-local env flag, I think it should only accept the public envs ( |
|
Thanks for the detailed review — you're right, the catch-all approach was too broad. The core problem with the previous fix was that it caught every [ConfigurationError] from [getConfigFileEffect] and treated it as "file not found". That silently swallowed malformed TOML, unreadable files, and schema validation failures — exactly what you called out. Here's what we changed: Resolve env first. init now calls [resolveEnvironmentEffect] upfront, using the same precedence rules as the rest of the CLI (env var overrides → ~/.gdenv → default [ote]. The raw [--environment] string is no longer passed directly into [getConfigFileEffect]. Check existence before reading. We use [fileExists] to determine whether the target config file is present. If it's absent and no explicit [--config] was passed, we skip cleanly. If it's absent and [--config] was explicitly provided, we fail hard. Fail hard on everything else. If the file exists, we read it and surface any error — bad TOML, unreadable, schema-invalid — without swallowing it. The [ArkErrors] path is now always checked, not just when [--config] was passed. On the broader design point about removing [--environment] from init: agreed it's confusing when lower-env overrides are already active, but treating that as a follow-up for now since it's a breaking change and the current fix already narrows the scope correctly. |
The bug was: passing [--environment prod] to [init] command triggered [getConfigFile()] which threw if no godaddy.toml existed — even though you'd already supplied all the flag values. Fix handles that.
Without [--environment], the code now skips the config file entirely and falls through to [resolveEnvironmentEffect(undefined)], which reads ~/.gdenv, not godaddy.toml. That's separate and expected.
Additional improvement:
Swapped [getConfigFile] import for [getConfigFileEffect] since [appInit] is inside [Effect.gen], it should use [getConfigFileEffect].