WinGetPackage DSC Resource #2863
Conversation
This comment has been minimized.
This comment has been minimized.
| [DSCResource()] | ||
| class WinGetPackage | ||
| { | ||
| [DscProperty(Key)] |
There was a problem hiding this comment.
Is there not additionally a way to specify that this value is required?
| { | ||
| $wingetPackageResource.Installed = $true | ||
| $wingetPackageResource.Version = $catalogPackage.InstalledVersion.Version | ||
| $wingetPackageResource.Source = $catalogPackage.DefaultInstallVersion.PackageCatalog.Info.Name |
There was a problem hiding this comment.
I'm not sure what you wanted the Source here to mean, but this likely isn't the correct value to pull.
If you want "the place that this came from", then $catalogPackage.InstalledVersion.PackageCatalog is more correct, but you would need to then determine how to handle the case that it was installed outside of winget.
|
|
||
| $catalogPackage = Get-WinGetPackage -Id $this.Id -Exact | ||
|
|
||
| # If no version is given, see if the latests is installed. |
There was a problem hiding this comment.
I would think that if no version is given, then it doesn't matter what version is present on the system. If you want to have Latest semantics, add a Latest property to set.
| # We can't have CatalogPackage as member of this class because it doesn't have a default constructor. | ||
| # A call to Invoke-DscResource will fail with | ||
| # 'ImportClassResourcesFromModule: Exception calling "ImportClassResourcesFromModule" with "4" argument(s): "The DSC resource 'CatalogPackage' has no default constructor."' | ||
| $catalogPackage = Get-WinGetPackage -Id $this.Id -Exact |
There was a problem hiding this comment.
Exact is not the correct matching strategy; it requires the case to match. What we want is EqualsCaseInsensitive, but that isn't exposed via PS currently. We alternate between that and ContainsCaseInsensitive in winget commands based on whether the command is targeting a single package or multiple, but the PS commands don't carry the actions that are intended during the search phase.
The matching strategy parameter needs to be changed to expose all of the options, probably as a parameter that takes an enumeration value.
|
|
||
| if ($this.Installed) | ||
| { | ||
| $catalogPackage = Get-WinGetPackage -Id $this.Id -Exact |
There was a problem hiding this comment.
This seems extremely inefficient. Couldn't you make an internal type/hashtable that contains all of the relevant information and get it just once?
|
|
||
| if ([string]::IsNullOrWhiteSpace($this.Version)) | ||
| { | ||
| Update-WinGetPackage @hashArgs |
There was a problem hiding this comment.
Again, I don't see how not specifying a version means "the latest version" rather than "I don't care what the version is".
| {'Greater' -or 'Unknown'} | ||
| { | ||
| # The installed package has a greated version or unknown. Uninstall and install. | ||
| Uninstall-WinGetPackage -Id $this.Id |
There was a problem hiding this comment.
We don't have a pipelineable parameter to just take in the package object? This call is not safe as written (ignores source).
microsoft-github-policy-service
bot
added
the
Needs-Author-Feedback
microsoft-github-policy-service
bot
removed
the
Needs-Author-Feedback
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| # DSC only validates keys and mandatories in a Set call. | ||
| if ([string]::IsNullOrWhiteSpace($this.Id)) | ||
| { | ||
| throw "WinGetPackage: Id is required" |
There was a problem hiding this comment.
//TODO: Localize this and other error messages
This is a preview implementation of a DSC resource that installs packages on winget.
For now, ensures a package is installed or not. Its install behavior depends if the package is already installed for the user or not.
This resource is not final and it just provides a simple implementation.
Microsoft Reviewers: Open in CodeFlow