Skip to content

Conversation

Gijsreyn
Copy link
Contributor

PR Summary

Implements an optimized approach to discovering PowerShell resources using .NET with parallel processing and direct result synchronization.

The previous approach using Get-ChildItem had unnecessary object overhead and required post-processing with Where-Object. The same is implied when using a thread-safe collection (ConcurrentBag), which adds unnecessary complexity.

PR Context

Fix #913

Note

While I recognize the need for caching in the issue, I left it out for now, assuming we can increment it over time.

@Gijsreyn
Copy link
Contributor Author

With PR #1025 around, it might be worthwhile to rename the directories.

@Gijsreyn Gijsreyn force-pushed the implement-powershell-discover branch 2 times, most recently from 623d593 to 5b7502f Compare August 24, 2025 21:04
@Gijsreyn Gijsreyn force-pushed the implement-powershell-discover branch from 5b7502f to 991f7ce Compare August 27, 2025 01:07
@Gijsreyn Gijsreyn force-pushed the implement-powershell-discover branch from 6337045 to f15195a Compare September 3, 2025 16:22
Copy link
Collaborator

@michaeltlombardi michaeltlombardi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few questions and requests:

  1. Do I correctly understand that this extension only for resources implemented in PowerShell and excluding Windows PowerShell? If so, do we need a second extension for discovering those resources, or can we handle that in this one? I'm not sure I see an immediate reason for why we can't handle both here, given the manifest has to indicate whether the resource is invoked with powershell or pwsh.
  2. Can we slightly restructure this script to define (currently empty) param block and put the implementation into the process block? That can help with organization and testing later on.
  3. The current implementation can't run in Windows PowerShell, I think, given the use of ForEach-Object -Parallel - but we can still discover resources in Windows PowerShell modules.

@Gijsreyn
Copy link
Contributor Author

Gijsreyn commented Sep 3, 2025

A few questions and requests:

  1. Do I correctly understand that this extension only for resources implemented in PowerShell and excluding Windows PowerShell? If so, do we need a second extension for discovering those resources, or can we handle that in this one? I'm not sure I see an immediate reason for why we can't handle both here, given the manifest has to indicate whether the resource is invoked with powershell or pwsh.
  2. Can we slightly restructure this script to define (currently empty) param block and put the implementation into the process block? That can help with organization and testing later on.
  3. The current implementation can't run in Windows PowerShell, I think, given the use of ForEach-Object -Parallel - but we can still discover resources in Windows PowerShell modules.
  1. The one currently written is only compatible with PowerShell 7+ because of the [System.IO.EnumerationOptions]. We might be able to rewrite it, but as you also mentioned (from point 3), the Foreach-Object -Parallel is not implemented. It would be best to create a second extension for it with different logic.
  2. Sure, I'll implement this change. Thanks for the comment!
  3. Answered in point 1.

param ()

begin {
$psPaths = $env:PSModulePath -split [System.IO.Path]::PathSeparator | Where-Object { $_ -notmatch 'WindowsPowerShell' }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we throwing away paths with WindowsPowerShell in them for performance reasons or because we don't want to support discovering resources shipped in Windows PowerShell modules?

If the latter, I think we might be better off retrieving the folder paths for discovered modules and searching those.

Copy link
Contributor Author

@Gijsreyn Gijsreyn Sep 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quite honestly, the only reason that I'm filtering it out is because if I can remember correctly, there was a bug in the past where we filtered out WindowsPowerShell in the adapter (or tests). Looking through the codebase, I can see this was the change: https://github.com/PowerShell/DSC/pull/777/files

If we opt for searching folder paths, I suppose it needs to match the specific operating systems, which becomes a ugly if/elseif/else block. It also limits custom installation, which will be lost. For example:

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Discover extension for PSModulePath
2 participants