-
Notifications
You must be signed in to change notification settings - Fork 52
Enable a single '.dsc.manifests.json' file to contain many resource/extension manifests #1187
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
base: main
Are you sure you want to change the base?
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 PR consolidates multiple resource manifests into a single .dsc.resourcelist.json
file, eliminating the need for separate files per resource. The change introduces support for resource list files that can contain an array of resource manifests, while maintaining backward compatibility with single-resource manifest files.
Key changes:
- Added
ResourceManifestList
struct to support arrays of resource manifests - Updated discovery logic to handle both single and list manifest files
- Consolidated 18 separate
dsctest
resource manifest files into onedsctest.dsc.resourcelist.json
Reviewed Changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
tools/dsctest/dsctest.dsc.resourcelist.json | New consolidated manifest file containing all dsctest resources |
tools/dsctest/*.dsc.resource.json (multiple) | Removed individual resource manifest files |
lib/dsc-lib/src/discovery/command_discovery.rs | Updated discovery and loading logic to support resource lists |
lib/dsc-lib/src/dscresources/resource_manifest.rs | Added ResourceManifestList struct |
lib/dsc-lib/src/dscerror.rs | Added InvalidManifest error variant |
lib/dsc-lib/src/extensions/discover.rs | Updated extension discovery to handle manifest lists |
build.ps1 | Updated file copy patterns to include resourcelist files |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
I'm going to update this PR based on recent discussion:
|
Do we need to have list in the name of the file? What about just having it be plural? Actually looks like that was the name of the file in the comment thread not sure why the new comment has a different name.
|
@ThomasNieto the original addition for |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
17a6be2
to
19cea7e
Compare
PR Summary
This change allows for a single
.dsc.manifests.json
(or yaml) file to contain multiple resource or extension manifests so you no longer need a separate file per resource/extension.No new test added as the
dsctest
resource now has a single manifest for all of its resources.This turned out to be more complicated than I expected due to a known limitation in
serde
for handlingHashMap<i32, String>
which was also embedded in anenum
which is now the case. To workaround this, the schema now hasexitCodes
beHashMap<String, String>
which is what it actually is in the JSON and at runtime, we parse the key to be a i32 otherwise return an error.Also some general code cleanup in the modified areas putting things in alpha order and modifying some code so that you get a specific error if there's invalid JSON/YAML instead of a generic error.
PR Context
FIx #1160