8000 Bind Dynamic Parameters Real-Time by dkattan · Pull Request #20069 · PowerShell/PowerShell · GitHub
[go: up one dir, main page]

Skip to content

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

Conversation

dkattan
Copy link
Contributor
@dkattan dkattan commented Aug 2, 2023

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:

Function Expand-ComputerName {
    [CmdletBinding()]
    Param()
    DynamicParam {
        Write-Host "Calling New-DynamicParameter for ComputerName"
        New-DynamicParameter -Name ComputerName -Mandatory -Type string
        Write-Host "`$ComputerName: $ComputerName"
        if ($ComputerName -like '*`$AssetTag*') {
            Write-Host "Calling New-DynamicParameter for AssetTag"
            New-DynamicParameter -Name AssetTag -Mandatory -Type string
            Write-Host "Done calling New-DynamicParameter for AssetTag"
        }
    }
    Process {
        $ExecutionContext.InvokeCommand.ExpandString($ComputerName)
    }
}

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

@dkattan
Copy link
Contributor Author
dkattan commented Aug 4, 2023

@iSazonov Curious to see your thoughts on this

@microsoft-github-policy-service
Copy link
Contributor

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

@microsoft-github-policy-service microsoft-github-policy-service bot added the Review - Needed The PR is being reviewed label Aug 12, 2023
@JamesWTruher JamesWTruher added WG-Engine core PowerShell engine, interpreter, and runtime WG-Engine-ParameterBinder labels Aug 17, 2023
@IISResetMe IISResetMe self-assigned this Aug 21, 2023
@dkattan
Copy link
Contributor Author
dkattan commented Aug 28, 2023

Any updates from the working group @JamesWTruher @IISResetMe ?

@iSazonov
Copy link
Collaborator

@iSazonov Curious to see your thoughts on this

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.

@microsoft-github-policy-service microsoft-github-policy-service bot removed the Review - Needed The PR is being reviewed label Aug 29, 2023
@dkattan
Copy link
Contributor Author
dkattan commented Aug 30, 2023

@iSazonov Curious to see your thoughts on this

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:
DynamicParameterBinding

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:

image

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
Loading

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 A parameter cannot be found that matches parameter name 'FirstName'

This is because $PrimaryUserOptions is defined in the dynamicparam block and its value has not yet been bound.

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 $DirectoryType and $PrimaryUserOptions into the static param block, but what if I decide to combine the functionality of this script with that of another with the use of my Get-CommandParameter function? This again shifts everything back into dynamicparams and I'm back to square one.

In conclusion I'd like this to be considered because

  1. It doesn't break anything
  2. It eliminates the need to duplicate paramblock dynamicparams blocks when aliasing or combining functions

@iSazonov
Copy link
Collaborator

@dkattan Thanks for the example. This demonstrates well the wrong approach.
As I said it's moving the logic into parameter binding. This leads to a number of problems. The main one is complexity. I wouldn't want to run into someone else's code that uses this.
Also the UX is very poor. It provokes writing code that may not work well on its own, and in addition it may have a bad effect on Engine. (For example, you already use Get-AzureADUsers for ValidateSet - what if it returns 100000 accounts?)
Plus, pseudo-binding won't work for cascaded dynamic parameters. We would also have to think about PowerShell providers. I don't see how it would work there.
You try to create elegant code, which is always what we all want to achieve, but in reality you create code that collects every conceivable and unthinkable problems.

@StartAutomating
Copy link

@iSazonov I can see a few other scenarios where this would be useful.

  1. Joining dynamic parameters of several commands.
  2. More succinctly filtering dynamic parameters
  3. (potentially) simplifying conflicts between dynamic parameters

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:

  • Attribution and additional metadata are valuable ways to enhance a cmdlet and make it look better in a UI.
  • The problems of binding PowerShell to any UI framework are painful and can be made much easier with small changes in the engine, such as this.
  • While PowerShell has lots of metadata, that metadata isn't always sufficient to provide a good user experience to a command on its own; it has to be augmented.
  • PowerShell providers have their own way of providing dynamic parameters, and it may benefit from a similar change.
  • Attributes can be easily stripped or added during the compilation/transpilation of a script (thus it need not clutter everyone's experience of reading the code)
  • Last, but not least, it looks like @dkattan put a lot of effort into the UX there, and I don't think it's helpful to insult someone's PowerShell+Web UX in a PR review.

FWIW, I think this would be a pretty useful change.

@dkattan
Copy link
Contributor Author
dkattan commented Aug 30, 2023

This demonstrates well the wrong approach.
As I said it's moving the logic into parameter binding. This leads to a number of problems. The main one is complexity. I wouldn't want to run into someone else's code that uses this.

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.

Also the UX is very poor. It provokes writing code that may not work well on its own, and in addition it may have a bad effect on Engine. (For example, you already use Get-AzureADUsers for ValidateSet - what if it returns 100000 accounts?)

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()]

Plus, pseudo-binding won't work for cascaded dynamic parameters. We would also have to think about PowerShell providers. I don't see how it would work there.

Pseudobinding does work as it calls the cmdletbinder under the hood, I tested it.

You try to create elegant code, which is always what we all want to achieve, but in reality you create code that collects every conceivable and unthinkable problems.

I disagree wholeheartedly. This encourages re-use of validation logic to ensure clean input.

@jsmithschilling
Copy link

As an ImmyBot user, I need this and love the UX.

@Dakota-LM
Copy link

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.

@bironeaj
Copy link

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.

@ryanleewilliams
Copy link

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.

@JerBar
Copy link
JerBar commented Aug 30, 2023

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.

@acmedevs
Copy link

As someone in a similar situation, I too would like to request this be given further consideration.

@MWGMorningwood
Copy link

As an ImmyBot user, the UX is extremely easy to grasp once you're in the right mindset.
The team at Immense are automation gods. If they're requesting new features, it's likely for the best.
Their use case of this specific PR is especially relevant to us.

@iSazonov
Copy link
Collaborator

@dkattan

Plus, pseudo-binding won't work for cascaded dynamic parameters. We would also have to think about PowerShell providers. I don't see how it would work there.

Pseudobinding does work as it calls the cmdletbinder under the hood, I tested it.

To be clear I don't mean this break pseudobinding, I mean pseudobinding cannot benefit from the enhancement.

You try to create elegant code, which is always what we all want to achieve, but in reality you create code that collects every conceivable and unthinkable problems.

I disagree wholeheartedly. This encourages re-use of validation logic to ensure clean input.

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.
Did I understand you correctly that you create a generic cmdlet with a full set of dynamic parameters and then read the meta information and dynamically build the Web interface?

@JerBar
Copy link
JerBar commented Aug 31, 2023

@dkattan

Plus, pseudo-binding won't work for cascaded dynamic parameters. We would also have to think about PowerShell providers. I don't see how it would work there.

Pseudobinding does work as it calls the cmdletbinder under the hood, I tested it.

To be clear I don't mean this break pseudobinding, I mean pseudobinding cannot benefit from the enhancement.

You try to create elegant code, which is always what we all want to achieve, but in reality you create code that collects every conceivable and unthinkable problems.

I disagree wholeheartedly. This encourages re-use of validation logic to ensure clean input.

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. Did I understand you correctly that you create a generic cmdlet with a full set of dynamic parameters and then read the meta information and dynamically build the Web interface?

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.

@IISResetMe
Copy link
Collaborator

The WG-Engine working group recently review this PR. As the current commits effectively cover two functional changes, I'll address them separately:

  • Allow dynamicparam to emit runtime parameters in a stream-like fashion
  • Dynamically re-bind parameters to allow for interdependent/cascading runtime parameter generation

Streaming output:

Review Verdict: approved, with comments

We recognize the frustration that the current rigid output requirements for the dynamicparam block poses, and welcome this suggested improvement.

In order to not break existing valid dynamicparam blocks, the implementation should observe the following rules:

  • If the blocks outputs a single RuntimeDefinedParameterDictionary instance, the existing behavior should persist and the dictionary should be used
  • If the output consists of 1 or more named RuntimeDefinedParameter instances, construct the dictionary from those

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-bind

Review 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 - dynamicparams is already too much of a "blackbox", and we'd like to ensure we don't perpetuate that state of affairs.

Before considering such a drastic change, we would like to see:

  • A more detailed problem statement (current limitations, core use cases, performance considerations, etc.)
  • Broader input from the community (additional use cases, discussion of additional risk factors)
  • A larger test suite

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.

@iSazonov iSazonov marked this pull request as draft September 4, 2023 14:09
@microsoft-github-policy-service microsoft-github-policy-service bot added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Sep 9, 2023
@StartAutomating
Copy link

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 - dynamicparams is already too much of a "blackbox", and we'd like to ensure we don't perpetuate that state of affairs.

A few notes here, from a concerned community member:

  1. As someone who uses dynamic parameters a lot, automatically populating the variable if it does not already exist would be huge. Despite writing a lot of dynamic parameter functionality and knowing "better", I always find myself presuming that the variable will be there, and then copying/pasting the same few lines that do the same thing as this PR (populate the variables in $psBoundParameters).

  2. Please don't underestimate the value of fixing this part of the ecosystem. Dynamic Parameters are incredibly powerful, especially when it command extensibility and reuse. Being able to "carry" parameters between one command and the next is very useful, and would get a lot easier with this PR (and easier still with a little more sweat)

  3. I agree that dynamicparams are too much of a black box. This is our opportunity to bring sunlight to that black box and make it much easier to work with. Even if we cannot illuminate the entire black box, clearly documenting how this part works would be a big benefit. As the saying goes, sunlight is the best disinfectant.

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.

@microsoft-github-policy-service microsoft-github-policy-service bot added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept labels Sep 25, 2023
StartAutomating pushed a commit to StartAutomating/PowerShell that referenced this pull request Sep 28, 2023
This helps exposes the current AST to various parts of PowerShell.
StartAutomating pushed a commit to StartAutomating/PowerShell that referenced this pull request Sep 28, 2023
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.
StartAutomating pushed a commit to StartAutomating/PowerShell that referenced this pull request Sep 28, 2023
…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.
StartAutomating pushed a commit to StartAutomating/PowerShell that referenced this pull request Sep 28, 2023
Validating normal dynamic parameter use, smart aliasing, emit, and conditional emit.
@microsoft-github-policy-service microsoft-github-policy-service bot removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Sep 28, 2023
@pull-request-quantifier-deprecated

This PR has 206 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!


Quantification details

Label      : Large
Size       : +119 -87
Percentile : 60.6%

Total files changed: 3

Change summary by file extension:
.cs : +105 -75
.ps1 : +14 -12

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

  • Fast and predictable releases to production:
    • Optimal size changes are more likely to be reviewed faster with fewer
      iterations.
    • Similarity in low PR complexity drives similar review times.
  • Review quality is likely higher as complexity is lower:
    • Bugs are more likely to be detected.
    • Code inconsistencies are more likely to be detected.
  • Knowledge sharing is improved within the participants:
    • Small portions can be assimilated better.
  • Better engineering practices are exercised:
    • Solving big problems by dividing them in well contained, smaller problems.
    • Exercising separation of concerns within the code changes.

What can I do to optimize my changes

  • Use the PullRequestQuantifier to quantify your PR accurately
    • Create a context profile for your repo using the context generator
    • Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
    • Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
    • Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
  • Change your engineering behaviors
    • For PRs that fall outside of the desired spectrum, review the details and check if:
      • Your PR could be split in smaller, self-contained PRs instead
      • Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

  • One line was added: +1 -0
  • One line was deleted: +0 -1
  • One line was modified: +1 -1 (git diff doesn't know about modified, it will
    interpret that line like one addition plus one deletion)
  • Change percentiles: Change characteristics (addition, deletion, modification)
    of this PR in relation to all other PRs within the repository.


Was this comment helpful? 👍  :ok_hand:  :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

@microsoft-github-policy-service microsoft-github-policy-service bot added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Oct 1, 2023
@dkattan
Copy link
Contributor Author
dkattan commented Oct 19, 2023

The WG-Engine working group recently review this PR. As the current commits effectively cover two functional changes, I'll address them separately:

  • Allow dynamicparam to emit runtime parameters in a stream-like fashion
  • Dynamically re-bind parameters to allow for interdependent/cascading runtime parameter generation

Streaming output:

Review Verdict: approved, with comments

We recognize the frustration that the current rigid output requirements for the dynamicparam block poses, and welcome this suggested improvement.

In order to not break existing valid dynamicparam blocks, the implementation should observe the following rules:

  • If the blocks outputs a single RuntimeDefinedParameterDictionary instance, the existing behavior should persist and the dictionary should be used
  • If the output consists of 1 or more named RuntimeDefinedParameter instances, construct the dictionary from those

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-bind

Review 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 - dynamicparams is already too much of a "blackbox", and we'd like to ensure we don't perpetuate that state of affairs.

Before considering such a drastic change, we would like to see:

  • A more detailed problem statement (current limitations, core use cases, performance considerations, etc.)
  • Broader input from the community (additional use cases, discussion of additional risk factors)
  • A larger test suite

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.

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.

@microsoft-github-policy-service microsoft-github-policy-service bot removed Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept Stale labels Oct 19, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Oct 23, 2023
@herringms
Copy link

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

@tylernocwing
Copy link

I hope this gets implemented dkattan

@dlfoster311
Copy link

I would love to see this implemented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Large Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept WG-Engine core PowerShell engine, interpreter, and runtime WG-Engine-ParameterBinder
Projects
None yet
Development

Successfully merging this pull request may close these issues.

0