-
Notifications
You must be signed in to change notification settings - Fork 7.7k
Bind Dynamic Parameters Real-Time #20069
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
Bind Dynamic Parameters Real-Time #20069
Conversation
@iSazonov Curious to see your thoughts on this |
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
Any updates from the working group @JamesWTruher @IISResetMe ? |
I tried to come up with a realistic scenario and failed miserably. In my opinion it is almost impossible (and useless) to use it in an interactive scenario. Especially when considering pipeline. As for scripts, it looks like we are moving conditions from outside to inside. We could see the benefit in that, although in my opinion it makes functions and cmdlets too complicated to understand and use. It would be useful to discuss a few real scenarios to see if we can use simpler and easier to understand alternatives. |
Hey @iSazonov thank you for checking it out! Let me start by showing you what I'm building: The left pane is essentially Show-Command but interactive and on the web. Note: This GIF is from the script editor/debugger interface. The end users see this form in the following context: The fact that I'm using a form necessitates that I collect all parameters from the user ahead of time. To prevent duplicating the extensive param blocks in the functions responsible for joining Active Directory, joining AzureAD, setting the computer name, and creating users in the respective directories, I instead created re-usable functions like Get-DirectoryTypeParameters that return RuntimeDefinedParameters. I also created a function called Get-CommandParameters that does the same thing in a more generic way. Function Get-CommandParameter
{
[Cmdletbinding()]
param(
[Parameter(Mandatory)]
[ValidateNotNullOrEmpty()]
[string]
$Name,
[Parameter(ValueFromRemainingArguments=$true)]
[Alias('Args')]
[AllowNull()]
[AllowEmptyCollection()]
$ArgumentList
)
$CommandMetadata = Get-Command -Name $Name -ErrorAction Stop -ArgumentList $ArgumentList
$CommonParameterNames = [System.Management.Automation.Internal.CommonParameters].GetProperties().Name
$CommandMetadata.Parameters.GetEnumerator() | ?{ $_.Key -notin $CommonParameterNames } | %{
$ParameterName = $_.Key
[System.Management.Automation.ParameterMetadata]$ParameterMetadata = $_.Value
New-Object -TypeName System.Management.Automation.RuntimeDefinedParameter($ParameterName, $ParameterMetadata.ParameterType, $ParameterMetadata.Attributes)
}
} After I created this PR, I discovered I'm not alone in my quest to reuse param block code. RobocopyPS implements New-ProxyFunction to reuse the extensive param block on Invoke-RoboCopy from functions like Copy-RoboItem and Sync-RoboItem. function Copy-RoboItem {
[CmdletBinding(SupportsShouldProcess = $True)]
param (
# Specifies the path to the source directory. Must be a folder.
[Parameter( Mandatory = $True,
ValueFromPipelineByPropertyName,
ValueFromPipeline)]
[Alias('Path', 'FullPath')]
[String[]]$Source
)
DynamicParam {
# Get available parameters from Invoke-RoboCopy and ignore parameters that is not for moving items
New-ProxyFunction -CommandName 'Invoke-RoboCopy' -CommandType 'Function' -ignoredParams "Source", "Purge", "Mirror", "MoveFiles", "MoveFilesAndDirectories"
} This works because Invoke-RoboCopy doesn't have dynamicparams. Specifically it doesn't have dynamic parameters that are conditionally present based on the value of a paramblock parameter. My specific use case is to have a single function that sets computer name, joins Active Directory or AzureAD, and creates a profile for the primary person. Naturally each of these tasks are implemented in their own functions complete with parameter validation, dynamic parameters, the works. Here's a visual: flowchart TD
subgraph "param()"
$DirectoryType
$UsernameTemplate
$ComputerNameTemplate
end
subgraph "dynamicparams{}"
$DirectoryType --> |AzureAD| $DeviceEnrollmentManagerUsername
$DirectoryType --> |AzureAD| $DeviceEnrollmentManagerPassword
$DirectoryType --> |ActiveDirectory| $DomainSelector
$DomainSelector --> $PrimaryUserOptions
$DirectoryType --> |ValidateSetValues| $UPNSuffix
$DirectoryType --> |ValidateSetValues| $PrimaryUser
$DeviceEnrollmentManagerUsername --> $PrimaryUserOptions
$DeviceEnrollmentManagerPassword --> $PrimaryUserOptions
$PrimaryUserOptions --> |None| End
$PrimaryUserOptions --> |NewUser| NewUser
subgraph NewUser
$UsernameTemplate --> |$FirstName.$LastName| UPNPrefixDefaultValue["Expand-String $UsernameTemplate"] --> |DefaultValue|$UPNPrefix
$UPNPrefix
$FirstName --> UPNPrefixDefaultValue
$LastName --> UPNPrefixDefaultValue
$UPNSuffix
end
end
$PrimaryUserOptions --> |ExistingUser| $PrimaryUser
$PrimaryUser --> End
NewUser --> End
Now, imagine I splatted the following Hashtable $ht = @{
DirectoryType = 'AzureAD'
DeviceEnrollmentManagerUsername = 'dem@contoso.com'
DeviceEnrollmentManagerPassword = 'hunter12'
PrimaryUserOptions = 'NewUser'
FirstName = 'John'
LastName = 'Doe'
} Without this change I get This is because New-DynamicParameter -Name PrimaryUserOptions -ValidateSet 'NewUser','ExistingUser','None' -Mandatory
switch($PrimaryUserOptions)
{
'NewUser' {
New-DynamicParameter -Name FirstName -Mandatory -Type string
New-DynamicParameter -Name LastName -Mandatory -Type string
}
'ExistingUser' {
switch($DirectoryType){
'ActiveDirectory' {
$Users = Get-ADUser | select -Expand DisplayName
}
'AzureAD' {
$Users = Get-AzureADUsers | select -Expand DisplayName
}
New-DynamicParameter -Name ExistingUser -ValidateSet $Users
}
} You could argue that I should move In conclusion I'd like this to be considered because
|
@dkattan Thanks for the example. This demonstrates well the wrong approach. |
@iSazonov I can see a few other scenarios where this would be useful.
This would simplify some existing code that uses dynamic parameters, such as that in RoughDraft, and the groundwork laid here would open up easier creation of dynamic parameters over time. As far as the other feedback:
FWIW, I think this would be a pretty useful change. |
I'm trying to move errors from runtime to invocation time. Garbage in, garbage out right? Validation logic should be complex to prevent issues down the line.
There's nothing stopping anyone from creating a ValidateSetAttribute with a string[100000] today. But to be fair, I did encounter this problem and worked around it using caching and pagination on a custom [DropdownAttribute()]
Pseudobinding does work as it calls the cmdletbinder under the hood, I tested it.
I disagree wholeheartedly. This encourages re-use of validation logic to ensure clean input. |
As an ImmyBot user, I need this and love the UX. |
Also an ImmyBot user, this UX is actually pretty good when compared to others I've encountered. Would love to have this functionality available, I can see it coming in handy for more than just the Configure Directory task. |
I think all of the above comments show there is a need for this in PowerShell. As a PowerShell developer and ImmyBot user, I can see this being extremely useful. @iSazonov, I'm not really sure why the insulting comment about the UI was necessary here. The GIF/screenshot was provided by @dkattan, as you requested a real-world scenario. This PR was opened to discuss a feature being added to PowerShell, not the UI of an unrelated application. This comment is a textbook example of a Microsoft code of conduct violation, which I'll be reporting. |
I can't say I understand all the implications of the change, but it doesn't sound like there are many downsides and it seems to open up a lot of possibilities. I know it's not worth much, but as someone who loves working with PowerShell and also loves working with ImmyBot, I'd like to see this change given further consideration. |
Hello, I use Immy very extensively, What Darren is doing here is awesome. We need this not just Directory migrations, but other configuration tasks that will prevent us from having several tasks to having 1 with proper parameters being supplied. As far as the UI, I haven't seen anyone doing anything this slick and this awesome. Immybot has saved me 100s of Engineering hours in less than 6 months of utilizing it. The UI is very intuitive and the entire ImmyBot Applications is one of the best UI/UX designs I've ever used. All the reasons why you say this should not be approved are purely opinion based. As far as complexity, show many any scripting platform or IDE that isn't massively complex, that is BS cop out. |
As someone in a similar situation, I too would like to request this be given further consideration. |
As an ImmyBot user, the UX is extremely easy to grasp once you're in the right mindset. |
To be clear I don't mean this break pseudobinding, I mean pseudobinding cannot benefit from the enhancement.
I'm not criticizing your product, much less you as some have written here. I am trying to understand how you benefit from this extension on your example. |
I would think that this is more in line with how PowerShell was envisioned to pass metadata back so you have better understanding of the objects you are manipulating between commands, very similar to how the Exchange power shell provider works. |
The WG-Engine working group recently review this PR. As the current commits effectively cover two functional changes, I'll address them separately:
Streaming output:Review Verdict: approved, with comments We recognize the frustration that the current rigid output requirements for the In order to not break existing valid
Should you wish to support scenarios deviating from those (eg. allowing mixed-type output and ignoring non-parameters), the functionality must be guarded by an experimental feature flag. Dynamic re-bindReview verdict: rejected While we do recognize that a limited number of use cases might benefit from this ability(and the currently suggested solution is clever), we also agree that support for cascading runtime parameter generation deviates too far from the original implementation of dynamic parameters to merit approval as a simple change - Before considering such a drastic change, we would like to see:
For these reasons, we recommend opening a separate issue to discuss what the implementation might look like and what the broader implications might be @dkattan I appreciate this is not the outcome you were hoping for - please continue sharing your thoughts on the topic as you already have in this discussion. Once we all have a good idea of exactly what limitations the current behavior imposes and what implications the suggested change actually has, we'll happily pick it up for review again. |
A few notes here, from a concerned community member:
I believe I will try to help improve the testability and transparency in this PR, as I think, done right it would be a "killer feature" for PowerShell Core. |
This helps exposes the current AST to various parts of PowerShell.
This helps exposes the current AST to various parts of PowerShell. It is the fastest way to give extra context to dynamic parameters, and will also likely be useful for other scenarios in which a callstack peek would prove too expensive.
…hell#20069) No longer automatically binding to results. Instead, providing additional context to the DynamicParam block in the form of $_ (the current command) and $input (the current CommandAST CommandElements). Also adding a fair amount of internal documentation to help clarify this part of the PowerShell codebase.
Validating normal dynamic parameter use, smart aliasing, emit, and conditional emit.
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
Since the name of this PR was focused on Real-Time binding, I created a separate PR for the approved part which is being able to declare dynamic parameters from a script without encapsulating them in a RuntimeDefinedParameterDictionary. |
From an MSP and partner perspective this feature would be huge for templating for all of our organizations. Internal MS teams often seem to be focused on enterprise and don't have the exposure necessary for this to make sense. Really appreciate your time revisiting this |
I hope this gets implemented dkattan |
I would love to see this implemented. |
PR Summary
The goal of this PR is to allow for more flexibility with dynamic parameters by binding RuntimeDefinedParameter objects as they are emitted from the dynamicparam block.
Assuming a helper function exists for creating RuntimeDefinedParameters, you will now be able to define dynamic parameters like this:
This doesn't break existing functionality as the change is limited to RuntimeDefinedPar 8000 ameter types.
PR Context
I am building a Web UI for running PowerShell scripts similar to Show-Command.
One of the limitations I encountered when building this UI is that dynamic parameters can only condition off the values of static parameters.
This is because the output of the dynamicParam block is collected and bound at the end.
This change uses the onDataAdded event to process individual RuntimeDefinedParameter objects as they are written. All other object types are accumulated and bound the same as before, preserving existing functionality.
The second notable change is the creation of variables for these newly bound parameters.
This is more of a quality of life change to improve the ergonomics of accessing dynamic parameter values.
Historically, you had to retrieve dynamic parameter values from $PSBoundParameters, which feels more like an oversight than an intentional design decision, but I digress.
While the instantiating of variables from string names is not my favorite paradigm as it makes static code analysis more difficult, it is a core PowerShell paradigm.
Invoke-WebRequest uses -SessionVariable and CommonParameters like OutVariable, ErrorVariable and so on create variables based on a specified name. I feel that when a dynamic parameter is found real-time, it makes sense to create a variable for it just like any other bound parameter. Digging into $PSBoundParameters is simply not ergonomic.
The argument could be made that the idea of binding parameters real-time goes against the paradigm of discoverability. But I would argue that when filling out a form visually, it is a lot less daunting to only see parameters that matter for your context, and to that end this facilitates the use of PowerShell.
PR Checklist
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
or[ WIP ]
to the beginning of the title (theWIP
bot will keep its status check atPending
while the prefix is present) and remove the prefix when the PR is ready.(which runs in a different PS Host).