Add support for remaining manifest v1.1 fields #1427
Conversation
| }, | ||
| "AppsAndFeaturesEntries": { | ||
| "$ref": "#/definitions/AppsAndFeaturesEntries" | ||
| }, | ||
| "Installers": { | ||
| "type": "array", |
There was a problem hiding this comment.
Should the length of the installers array be bumped to 1024 here, per #967? #Resolved
There was a problem hiding this comment.
Yes, thanks for catching that!
| "type": [ "boolean", "null" ], | ||
| "description": "Indicates whether the installer requires an install location provided" | ||
| }, | ||
| "RequireExplicitUpgrade": { |
There was a problem hiding this comment.
I think we need an enum for that, but I cannot remember exactly the reasoning for that. But at least at a minimum there are both elevation required installers and elevation rejecting installers. So it would be better to have an enum value that we can set ElevationBehavior or something like that.
There was a problem hiding this comment.
More thoughts on values for it:
ElevationRequired // intended to indicate that the caller must be elevated because the installer will not elevate itself
ElevatesSelf
RestrictsSelf // Better word than restrict?
RestrictedRequired
Then we could add more as we find need of them. For instance ElevationRequiredForSilent was a problem for MSI before we fixed it, but I know that for instance WebView2 falls into this category currently. It self elevates for its normal unattended pattern, but does not for a silent install (basically same as msiexec behavior).
There was a problem hiding this comment.
Per offline chat, we'll do an enum of elevationRequired, elevationProhibited and elevatesSelf
There was a problem hiding this comment.
Didn't look at REST code; letting @ashpatil-msft look at that part.
|
|
||
| if (manifestVersion >= ManifestVer{ s_ManifestVersionV1_1 }) | ||
| { | ||
| std::vector<FieldProcessInfo> fields_v1_1 = |
There was a problem hiding this comment.
I don't see anywhere that we are limiting what the server will accept. Letting manifests into winget-pkgs when the feature behind them has not yet been implemented will likely lead to failures. Either now because someone relies on it for a package and it doesn't actually do anything or in the future when we start using it and find that we wanted validation to prevent some case that is now already present.
It feels like we should have:
- Add new version to client to allow for local testing; full validation fails if any 1.1 fields are present even if manifest is 1.1
- As we add functionality and validation, we start allowing fields that are ready #Resolved
There was a problem hiding this comment.
Evaluated 1 by 1 offline and we're ok with them for now
| constexpr std::string_view Markets = "Markets"sv; | ||
| constexpr std::string_view AllowedMarkets = "AllowedMarkets"sv; | ||
| constexpr std::string_view ExcludedMarkets = "ExcludedMarkets"sv; | ||
| constexpr std::string_view ElevationRequirement = "ElevationRequirement"sv; |
There was a problem hiding this comment.
Yes, this(From comments above) and expected return codes(Flor's) are 2 new fields added after our v1.1 schema review meeting, we'll have to update the rest spec later.
| installer.Markets.ExcludedMarkets = V1_0::Json::ManifestDeserializer::ConvertToManifestStringArray( | ||
| JsonHelper::GetRawStringArrayFromJsonNode(marketsNode.value().get(), JsonHelper::GetUtilityString(ExcludedMarkets))); | ||
| installer.Markets.AllowedMarkets = V1_0::Json::ManifestDeserializer::ConvertToManifestStringArray( | ||
| JsonHelper::GetRawStringArrayFromJsonNode(marketsNode.value().get(), JsonHelper::GetUtilityString(AllowedMarkets))); |
There was a problem hiding this comment.
Do we throw error if both are given?
There was a problem hiding this comment.
Yes, it's in the manifestvalidation.cpp
|
Related to #1243 |
Change
Validation
Added unit tests and also tested manually.
Microsoft Reviewers: Open in CodeFlow