Refactor settings management and tracing configuration#1617
Open
Gijsreyn wants to merge 6 commits into
Open
Conversation
Contributor
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() {
Collaborator
|
@Gijsreyn - taking a look at this now. I may adopt the PR to make some changes, but will comment first. |
Collaborator
Author
|
@michaeltlombardi - copy that sir! |
94a4642 to
5ce9a97
Compare
…s://github.com/Gijsreyn/operation-methods into PowerShellgh-1081/main/strongly-typed-settings
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.
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).