fix(cli): load config in Ignore mode for auto-config-simulate - #3795
fix(cli): load config in Ignore mode for auto-config-simulate#3795Akbar Dizaji (AkbarDizaji) wants to merge 3 commits into
Conversation
…3791) TrySimulateAutoentities built its DeserializationVariableReplacementSettings without an envFailureMode, so it defaulted to Throw. `dab init` always writes three OpenTelemetry @env() placeholders (OTEL_EXPORTER_OTLP_ENDPOINT, OTEL_EXPORTER_OTLP_HEADERS, OTEL_SERVICE_NAME) into the generated config, and those variables are normally unset. Deserialization therefore threw and the command aborted with "Failed to read the config file" on a config that `dab start` loads and serves without complaint. Ignore is the correct mode because it is what the engine uses: every engine load goes through TryLoadKnownConfig, which hardcodes Ignore, so unresolved placeholders are tolerated by design. Simulation inspects the same config the engine would run, and must not reject configs the engine accepts. The underlying reason was also invisible. The loader buffers its logs until a logger is attached, and only the `start` verb attaches one, so the parse error was dropped and only the generic message remained. Attach a logger and drain the buffer on the failure path, then suppress the generic message when IsParseErrorEmitted reports the detailed error was already emitted. Because Ignore lets an unresolved reference survive into the connection string, where it would be sent to the database as a literal, reject a connection string still containing @env(' or @akv(' with an actionable message. Tests now assert the specific error rather than a bare IsFalse, which is what lets them distinguish a config-load failure from the check under test, and cover both an unset telemetry placeholder and the connection-string guard. The test loader is constructed with isCliLoader, matching how the CLI builds it; without it a successful load starts a hot-reload watcher over the mock file system whose IO retries add seconds to every test.
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
This PR fixes dab configure --auto-config-simulate config-loading behavior so it accepts the same configs the engine (dab start) accepts, particularly when generated configs contain unresolved telemetry @env() placeholders, and it improves error surfacing during config parse/load failures.
Changes:
- Load configs for auto-config simulation with environment-variable replacement in
Ignoremode to tolerate unresolved placeholders that the runtime engine already tolerates. - Flush buffered loader logs on config-load failure and avoid printing a generic error when a detailed parse error was already emitted.
- Add a guard that fails fast with an actionable message when the connection string still contains unresolved
@env()/@akv()references; expand unit test coverage accordingly.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Cli/ConfigGenerator.cs | Adjusts simulation config-loading failure mode, improves failure logging, and adds a connection-string unresolved-placeholder guard. |
| src/Cli.Tests/AutoConfigSimulateTests.cs | Adds regression/unit tests for telemetry-placeholder tolerance and the new connection-string guard; aligns loader construction with CLI usage. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| [TestInitialize] | ||
| public void TestInitialize() | ||
| { | ||
| _fileSystem = FileSystemUtils.ProvisionMockFileSystem(); | ||
| _runtimeConfigLoader = new FileSystemRuntimeConfigLoader(_fileSystem); | ||
| // isCliLoader mirrors how the CLI builds its loader. Without it a successful load starts a | ||
| // hot-reload file watcher against the mock file system, whose retries add seconds per test. | ||
| _runtimeConfigLoader = new FileSystemRuntimeConfigLoader(_fileSystem, isCliLoader: true); |
There was a problem hiding this comment.
Good catch — fixed in 6277bed.
The three OTEL_* names are the real ones, so this could not be dodged by using fake names the way RuntimeConfigLoaderTests does: the config under test is generated by dab init, which hardcodes them. Instead the class now snapshots the host values of every variable it mutates (the three OTEL_* plus DAB_TEST_UNSET_CONNECTION_SECRET) in TestInitialize and restores them in TestCleanup, so the cleanup covers every test in the class rather than relying on each one to remember.
| // When IsParseErrorEmitted is true, TryLoadConfig already emitted the | ||
| // detailed error to Console.Error. Only log a generic message to avoid | ||
| // duplicate output (stderr + stdout). | ||
| if (!loader.IsParseErrorEmitted) | ||
| { | ||
| _logger.LogError("Failed to read the config file: {runtimeConfigFile}.", runtimeConfigFile); | ||
| } |
There was a problem hiding this comment.
Correct, and reachable — fixed in b90200d.
TryGetConfigFileBasedOnCliPrecedence deliberately does not check that a user-provided path exists ("The existence of user provided config file is not checked here"), so -c missing.json reaches TryLoadConfig, which logs Unable to find config file: ... does not exist. without setting IsParseErrorEmitted. Before this PR the message was dropped with the rest of the buffer, so the duplicate is something the flush newly exposed.
The fallback is now gated on the file existing as well. The loader attributes both failures it can explain — a parse error via IsParseErrorEmitted, a missing file via its own message — so the generic message is only needed for the one case it stays silent on: a config file that exists but is empty. Added TestSimulateAutoentities_MissingConfigFile_DoesNotLogGenericError, which is also the first test to exercise the SetLogger/FlushLogBuffer path.
|
@microsoft-github-policy-service agree |
ClearOpenTelemetryEnvironmentVariables unset the three real OTEL_* names and never put them back, and the connection-string guard test unset DAB_TEST_UNSET_CONNECTION_SECRET the same way. MSTest runs an assembly's tests in one process, so a cleared variable leaked into every test that ran later, making unrelated tests pass or fail depending on ordering and on whether the host had OTEL_* set. The real names are unavoidable here: the config under test is produced by `dab init`, which hardcodes them, so this cannot be dodged by using fake names the way RuntimeConfigLoaderTests does. Snapshot the values in TestInitialize and restore them in TestCleanup instead, which covers every test in the class rather than relying on each one to clean up after itself.
Draining the loader's log buffer on the failure path made the loader's own error visible, which exposed that the generic fallback then repeats it. TryGetConfigFileBasedOnCliPrecedence deliberately does not check that a user-provided path exists, so `--auto-config-simulate -c missing.json` reaches TryLoadConfig, which logs "Unable to find config file: missing.json does not exist." without setting IsParseErrorEmitted. The flush delivered that, and then "Failed to read the config file: missing.json." followed it. Gate the fallback on the file existing as well. The loader attributes both failures it can explain - a parse error via IsParseErrorEmitted, a missing file via its own message - so the fallback is only needed for the one case it stays silent on, a config file that exists but is empty. The new test is the first to exercise the flush path, so it also covers the SetLogger/FlushLogBuffer call itself.
Why make this change?
Closes #3791
dab configure --auto-config-simulate(TrySimulateAutoentities) fails withFailed to read the config fileon a config thatdab startloads and serves without complaint.The cause is the failure mode used when loading the config.
TrySimulateAutoentitiesbuilt itsDeserializationVariableReplacementSettingswithout anenvFailureMode, so it defaulted toThrow.dab initalways writes three OpenTelemetry@env()placeholders into the generated config (OTEL_EXPORTER_OTLP_ENDPOINT,OTEL_EXPORTER_OTLP_HEADERS,OTEL_SERVICE_NAME), and those variables are normally unset — so deserialization threw and the command aborted on a perfectly runnable config.The underlying reason was also invisible to the user. The loader buffers its logs until a logger is attached, and only the
startverb attaches one, so the real parse error was dropped and only the generic message survived.What is this change?
1. Load in
Ignoremode.Ignoreis what the engine itself uses: every engine load goes throughTryLoadKnownConfig, which hardcodesIgnore, so unresolved placeholders are tolerated by design. Simulation inspects the same config the engine would run, and so must not reject configs the engine accepts.2. Surface the actual parse error. On the failure path, attach a logger and drain the loader's log buffer, then suppress the generic
Failed to read the config filemessage whenIsParseErrorEmittedreports that the detailed error was already written to stderr (avoids duplicate output across stderr + stdout).3. Guard the connection string. Because
Ignorelets an unresolved reference survive into the connection string — where it would be sent to the database as a literal — explicitly reject a connection string still containing@env('or@akv(', with an actionable message instead of a driver-level failure.Net effect:
auto-config-simulatenow accepts exactly the configs the engine accepts, and when a config genuinely is malformed the user sees why.How was this tested?
src/Cli.Tests/AutoConfigSimulateTests.cs:IsFalse. This is what lets them distinguish an incidental config-load failure from the check actually under test — without it, the bug in this PR was passing the suite.@env()placeholder (must now succeed) and for the connection-string placeholder guard (must fail with the actionable message).isCliLoader, matching how the CLI builds it. Without it, a successful load starts a hot-reload watcher over the mock file system whose IO retries add seconds to every test.Sample Request(s)
Before — on a stock
dab initconfig with the OTEL variables unset:...while the same config starts fine:
$ dab start # loads and serves without complaintAfter — the same command succeeds, and a genuinely broken config reports the reason:
Connection string with an unresolved reference: