-
Notifications
You must be signed in to change notification settings - Fork 55
Enable resource manifests to have relative paths #1224
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
Conversation
|
Tested with |
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 PR adds support for relative path resolution for executables in DSC resource manifests. When a resource manifest specifies an executable with a relative path, the code now resolves it relative to the manifest's directory using path canonicalization.
Key changes:
- Added
canonicalize_whichutility function to resolve executable paths relative to a working directory - Updated
invoke_commandto canonicalize executables before execution - Modified
verify_executablein discovery to check relative paths against manifest directories
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/dsc-lib/src/util.rs | Added canonicalize_which function to resolve relative executable paths and added necessary imports |
| lib/dsc-lib/src/dscresources/command_resource.rs | Updated invoke_command to use canonicalize_which for path resolution |
| lib/dsc-lib/src/dscerror.rs | Added CommandNotFound error variant (appears unused in the changes) |
| lib/dsc-lib/src/discovery/command_discovery.rs | Updated verify_executable to accept directory parameter and use canonicalize_which for validation; cleaned up imports |
| lib/dsc-lib/locales/en-us.toml | Added localized error message for executable not found scenario |
| dsc/tests/dsc_discovery.tests.ps1 | Added test case for relative path resolution in resource manifests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
andyleejordan
left a comment
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'm on vacation but can check back on Tuesday to review again 👍
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
3e096da to
79c27ea
Compare
andyleejordan
left a comment
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 I'd want to double check using canonicalize over which_in given the potential UNC issues, but up to your discretion. This otherwise looks good to me, though please file an issue for the Unicode path bug we probably have.
|
@andyleejordan created #1233 to track using I appreciate you digging into the details here. In this case, the result is only ever used by |
PR Summary
Hit a partner scenario where they don't want to have all of their assets in
PATH, so this change enables a resource manifest to specify the executable with a relative path so that only the resource manifest is found inPATH. This change modifies both the verification of the exe and also the invocation of the exe. If the specified executable has a relative path, then the path is resolved with the directory of the location of the resource manifest.The major change here is that the
verify_executable()helper needs the current directory to use a new helpercanonicalize_which()that checks if the specified exe in the manifest exists or is a relative path. If relative, it canonicalizes the joined path with the current directory and also checks on Windows that if the exe doesn't have an extension (since it could be.cmd,.bat, etc...), then it adds.exewhich is required for thecanonicalize()function which fails if the resulting path (with extension) doesn't exist.Some minor code cleanup with
std::fsAPI usage to make it consistent.