Repository navigation
(GH-1735) Fix directives version against running dsc version - #1744
Gijs Reijn (Gijsreyn) wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The implementation consistently fixes both CLI entry points and provides appropriate regression coverage.
0 open findings
What changed in this PR
Fixes DSC configuration version validation to use the running executable’s version rather than dsc-lib’s crate version.
Changes:
- Adds a version-aware
Configuratorconstructor. - Handles prerelease versions according to the documented matching rules.
- Updates CLI/MCP callers and adds Pester coverage.
| File | Description |
|---|---|
lib/dsc-lib/src/configure/mod.rs |
Validates directives using the supplied DSC version. |
dsc/src/subcommand.rs |
Passes the CLI version into configuration processing. |
dsc/src/server/invoke_dsc_config.rs |
Passes the MCP server’s DSC version. |
dsc/tests/dsc_version.tests.ps1 |
Tests running-version and prerelease matching. |
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
Mikey Lombardi (He/Him) (michaeltlombardi)
left a comment
There was a problem hiding this comment.
I added a few notes for maintainability/validation.
I think we should also consider defining a helper function in dsc to retrieve and parse the current version from env!("CARGO_PKG_VERSION") that panics on invalid semver parsing. Something like
/// Returns the current version of DSC as a semantic version.
///
/// # Returns
///
/// - The `SemanticVersion` for DSC if correctly defined in the
/// cargo manifest.
///
/// # Panics
///
/// If the defined version in the cargo manifest isn't a valid
/// semantic version, this function panics and reports the parse
/// error.
pub(crate) fn current_dsc_version() -> SemanticVersion {
let manifest_version = env!("CARGO_PKG_VERSION");
match SemanticVersion::parse(manifest_version) {
Ok(v) => v,
Err(e) => panic!("unable to parse {manifest_version} as semver: {e:#?}"),
}
}If we want to be really clever, we could probably figure out how to generate the current version of DSC as a static or constant in dsc-lib with a build script that pulls the manifest version from dsc. Then we wouldn't need to pass anything from dsc to dsc-lib and we could keep the same public API.
3b1e688 to
8982e57
Compare
…://github.com/Gijsreyn/operation-methods into PowerShellgh-1735/main/fix-directives-version
…dation - Apply standard semantic version rules to prerelease builds - Store the DSC version as a parsed SemanticVersion on the context - Take SemanticVersion in Configurator::new_with_dsc_version() - Add current_dsc_version() helper to dsc - Document Configurator::new_with_context() - Cover prerelease matching rules in the Pester tests
|
Thanks for the review, Mikey. All four points are addressed in the latest push. Per thread: Prerelease special case in
Reference docs on
|
PR Summary
This change:
directives.versionrequirement of a configuration document against the version of DSC that is processing it, instead of the version of thedsc-libcrate.Configurator::new_with_dsc_version()constructor, which sets the DSC version on the context before validating the configuration.Configurator::new()keeps its signature and still falls back to thedsc-libcrate version.dsc config *subcommands and the MCP serverinvoke_dsc_configtool to use the new constructor.versionNotSatisfiederror message.directives.versionis checked against dsc-lib's crate version (3.2.0), not the running dsc version #1735PR Context
Prior to this change,
validate_config()parsedenv!("CARGO_PKG_VERSION")to get the version to compare against thedirectives.versionrequirement. Insidedsc-lib, that macro resolves to the version of thedsc-libcrate, not the version of thedscCLI. The two versions aren't kept in sync, so on DSC 3.3.0 the requirement=3.3.0failed while=3.2.0and<3.3.0passed.The CLI did set
context.dsc_versionto its own version, but only afterConfigurator::new()returned. Becausenew()callsvalidate_config(), the value wasn't available yet when the directive was checked. The result metadata already usedcontext.dsc_version, which is why the output reported the correct version even though the directive was checked against the wrong one.The existing tests used the requirements
=999.0.0and>=3.1, which give the same result for either version, so they didn't catch the problem.Remark on preview builds
Using the real DSC version exposes a side effect of semantic version matching: a comparator only matches a prerelease version when the comparator itself defines a prerelease segment. With strict matching,
3.4.0-preview.2doesn't satisfy>=3.1, so every preview build would reject configuration documents with an ordinary version requirement. This went unnoticed before because thedsc-libcrate version never had a prerelease segment.To avoid that regression, this change compares a prerelease build of DSC by its release version (
3.4.0for3.4.0-preview.2) unless a comparator in the requirement explicitly defines a prerelease segment. In that case the full version is compared with the normal semantic version rules.The following table shows the results for DSC
3.4.0-preview.2:directives.version>=3.1=3.4.0<3.4.0=3.4.0-preview.2=3.4.0-preview.1>=3.4.0-preview.1>=3.4.0-preview.3=3.2.0<3.3.0The tradeoff is that
=3.4.0is satisfied by a3.4.0preview build, and<3.4.0isn't, even though a preview sorts before its release. If strict semantic version matching is preferred, the prerelease handling invalidate_config()can be removed, but then the existing>=3.1test fails on preview builds and the new prerelease test cases need to change.