Refactor settings management and tracing configuration#1617
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new strongly-typed settings layer in dsc-lib that resolves DSC settings once per invocation (with defined precedence across built-in, install, user, workspace, and policy scopes), and updates resource discovery and tracing configuration to consume those resolved settings.
Changes:
- Removed the legacy stringly-typed
get_setting()implementation and related localization strings. - Added
dsc-lib::settingsmodule with typed settings structs, JSON Schema derivations, and read-once caching (get_settings()). - Updated command discovery and CLI tracing to use resolved settings; expanded Pester coverage for settings precedence and XDG-based user settings.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/dsc-lib/src/util.rs | Removes get_setting() and policy-path helper in favor of centralized settings resolution. |
| lib/dsc-lib/src/settings/mod.rs | New typed settings module: resolution logic, scope tracking, file discovery, caching, and unit tests. |
| lib/dsc-lib/src/lib.rs | Exposes the new settings module publicly. |
| lib/dsc-lib/src/dscresources/command_resource.rs | Extends TraceLevel to support serialization + JSON Schema derivation for settings/schema use. |
| lib/dsc-lib/src/discovery/command_discovery.rs | Switches resource path resolution to get_settings() (resource discovery now settings-driven). |
| lib/dsc-lib/locales/en-us.toml | Removes old util/discovery setting messages; adds new settings.* messages. |
| dsc/tests/dsc_settings.tests.ps1 | Adds precedence tests for user/workspace/policy scopes; preserves/restores XDG_CONFIG_HOME. |
| dsc/src/util.rs | Switches tracing setup to resolved settings (get_settings()) rather than ad-hoc per-call file reads. |
| dsc/src/args.rs | Re-exports TraceFormat from dsc-lib::settings to keep a single definition. |
| dsc/locales/en-us.toml | Removes unused tracing-setting read error message. |
Comments suppressed due to low confidence (1)
dsc/src/util.rs:327
DSC_TRACE_LEVELis applied before checking whether the tracing setting came from policy. If policy definestracing(even withallowOverride: true), a user can still override it viaDSC_TRACE_LEVEL, which undermines the guarantee that policy-scoped fields are not overridable.
// override with DSC_TRACE_LEVEL env var if permitted
if tracing_setting.allow_override && let Ok(level) = env::var(DSC_TRACE_LEVEL) {
tracing_setting.level = match level.to_ascii_uppercase().as_str() {
|
@Gijsreyn - taking a look at this now. I may adopt the PR to make some changes, but will comment first. |
|
@michaeltlombardi - copy that sir! |
94a4642 to
5ce9a97
Compare
…s://github.com/Gijsreyn/operation-methods into PowerShellgh-1081/main/strongly-typed-settings
michaeltlombardi
left a comment
There was a problem hiding this comment.
A couple of design considerations - I'm happy to adopt this if you'd like or continue to provide async feedback.
| #[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] | ||
| #[serde(rename_all = "camelCase")] | ||
| pub enum SettingsScope { | ||
| /// The code default; the field isn't defined in any settings file. | ||
| Default, | ||
| /// The built-in defaults file next to the executable. | ||
| BuiltIn, | ||
| /// The install settings file next to the executable. | ||
| Install, | ||
| /// The user settings file. | ||
| User, | ||
| /// The workspace settings file in the current directory. | ||
| Workspace, | ||
| /// The machine policy file. Fields defined as policy cannot be overridden. | ||
| Policy, | ||
| } |
There was a problem hiding this comment.
I think this actually misses two scopes - Environment and CommandLine - where environment overrides workspace and command line options override environment (and all the way up the chain).
We want to capture those values as well so we can effectively implement a dsc settings show command that provides a view of how the settings resolved from which sources, including specifying the environment variables and CLI options, like
DSC_TRACE_LEVEL=INFO dsc --trace-format json settings showtracing:
level: info
format: json
allowEnvOverride: true
resource_path:
allowEnvOverride: true
appendEnvPath: true
directories: []DSC_TRACE_LEVEL=INFO dsc --trace-format json settings show --with-sourcestracing:
level:
value: info
scope: environment
format:
value: json
scope: cli
allowEnvOverride:
value: true
scope: default
resource_path:
allowEnvOverride:
value: true
scope: default
appendEnvPath:
value: true
scope: default
directories:
value: []
scope: default| /// The fully-resolved settings for the current invocation. | ||
| #[derive(Clone, Debug, PartialEq, Serialize, Deserialize, JsonSchema)] | ||
| #[serde(rename_all = "camelCase")] | ||
| pub struct ResolvedSettings { | ||
| /// The resolved tracing setting. | ||
| pub tracing: ResolvedField<TracingSetting>, | ||
| /// The resolved resource path setting. | ||
| pub resource_path: ResolvedField<ResourcePathSetting>, | ||
| } |
There was a problem hiding this comment.
The main problem I see with this implementation is that we're considering the container to be a single resolved field. Instead, we should have a type that represents the resolved containers where every leaf item is a resolved field.
This is a bit messy to maintain by hand but I think we can manage the complexity with declarative macros initially and later on with proc macros.
Motivation
As described in #1081, DSC settings are currently retrieved through stringly-typed
get_setting()function. This makes the available settings undiscoverable without tracing code flow, nor does it provide JSON Schema for users (or integration tooling). This PR implements the core settings module agreed upon in the issue so settings are centrally defined + strongly typed.Approach
This POR adds a new
settingsmodule indsc-lib, defining every setting DSC supports and resolving them once per invocation. The resolution follows the agreed WG model: sources are gathered from lowest to highest precedence. For example:If
resourcePathis defined in both the user and workspace files, the workspace value winsKey design decisions made:
ResolvedSettings::resolve())get_settingsand then results inLazyLock<RwLock<Option<Arc<...>>>>Other side notes: this PR is the foundational layer for two open issues (#894 and #545).