-
Notifications
You must be signed in to change notification settings - Fork 34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix not parsing expressions for Group resources #695
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request fixes the issue where expressions were incorrectly parsed and invoked for Group resources by bypassing the expression invocation for such cases. The changes ensure that Group resources handle their own expression parsing while other resource kinds continue to invoke expressions as before.
- Updated the resource handling in dsc_lib/src/configure/mod.rs to check for Group resource types and skip expression invocation.
- Modified discovery usage by cloning the Discovery instance to avoid borrow conflicts, and added a Clone derive for Discovery in dsc_lib/src/discovery/mod.rs.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
dsc_lib/src/configure/mod.rs | Updated logic in multiple resource functions to bypass expression parsing for Group types and to use a cloned discovery instance. |
dsc_lib/src/discovery/mod.rs | Added #[derive(Clone)] on Discovery to support cloning for borrow resolution. |
Comments suppressed due to low confidence (1)
dsc_lib/src/discovery/mod.rs:12
- [nitpick] Deriving Clone for Discovery is necessary for the fix; however, ensure that cloning the Discovery does not incur excessive overhead, especially if the resources map becomes large.
#[derive(Clone)]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be worth using a helper function to get the resource properties since it's used across get, set, test, and export?
1fb3acb
to
f3d2a5a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will leave the same class of problem for adapted resources - and for any groups that are externally implemented (i.e. not recursively calling dsc
) I think this ends up requiring those resources to handle all expression resolution, right?
I wonder if there's a coherent way (in a future PR) to differentiate between expressions that can be pre-resolved and those that the resource needs to resolve itself (or, better yet, ask DSC to resolve somehow, but I can't think of any non-complex way to even start on that).
For right now, I think this PR makes sense and immediately helps people using the existing group resources, so these comments/thoughts are non-blocking.
@@ -79,4 +79,32 @@ string. | |||
"@.Replace("`r", "") | |||
$out.results[1].result.actualState.output | Should -BeExactly "This is a single-quote: '" | |||
} | |||
|
|||
It 'Nested Group resource does not invoke expressions' { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this interact with expressions that need the top-level document to resolve, like retrieving a configuration parameter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already have this issue #381, so it's currently not possible
PR Summary
THe previous logic was to recursively parse and invoke expressions to handle the case of a deeply nested object that used an expression. The problem is that when a Group resource is encountered, this logic would still apply and fail because execution had not happened yet.
The fix is for the configuration to NOT parse and invoke expressions if the resource kind is a Group as when that Group resource is invoked, it will handle parsing and invoking expressions it contains within its scope.
As part of the fix, need to clone the
discovery
member to invoke it to get theDscResource
to find out the kind as this would be an immutable borrow ofself
which would cause the later mutable borrow ofself
to fail. So need to deriveclone()
forDiscovery
for this to work.PR Context
Fix #692