-
Notifications
You must be signed in to change notification settings - Fork 42
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
Enable parameter injection for Azure PowerShell response #339
Conversation
@wangzelin007 can you please also review the changes? I cannot add you directly as a reviewer yet as you haven't accept the invite to become a contributor to this repo. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 5 out of 9 changed files in this pull request and generated no comments.
Files not reviewed (4)
- build.psm1: Language not supported
- shell/AIShell.App/AIShell.App.csproj: Language not supported
- shell/agents/Microsoft.Azure.Agent/Microsoft.Azure.Agent.csproj: Language not supported
- shell/agents/Microsoft.Azure.Agent/AzureAgent.cs: Evaluated as low risk
Comments suppressed due to low confidence (3)
shell/agents/Microsoft.Azure.Agent/DataRetriever.cs:389
- [nitpick] The variable 'variable' is declared but not always used, which might lead to confusion. Consider utilizing it more clearly or removing it if not necessary.
VariableExpressionAst variable = null;
shell/agents/Microsoft.Azure.Agent/DataRetriever.cs:668
- The 'returnValues' list is initialized multiple times within the same method, which is inefficient. Consider initializing it once and reusing it.
List<string> returnValues = null;
shell/agents/Microsoft.Azure.Agent/DataRetriever.cs:654
- Ensure that all potential null values are adequately checked, especially in methods like 'GetArgValuesForAzPS'.
if (!cmdInfo.Parameters.TryGetValue(parameter, out ParameterMetadata paramMetadata))
LGTM~ |
1 similar comment
LGTM~ |
|
||
// Today, Azure PowerShell's argument completer attributes don't use 'CommandAst' and the fake bound parameters. | ||
// So we pass 'null' for both of them for simplicity. | ||
IEnumerable<CompletionResult> results = completer?.CompleteArgument(command, parameter, "", null, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we plan to implement the wordToComplete
in phase 2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The wordToComplete
is an empty string, so it matches everything.
Did you mean the "CommandAst" and "fakeBoundParameters" parameters? They are not used by all existing completer attributes in AzPS today, so I didn't bother passing them in. It's easy to get the CommandAst
, but not for the fakeBoundParameters
.
PR Summary
This PR enables parameter injection for responses from the Azure PowerShell handler.
AssemblyLoadContext
, while PowerShell runtime has to live in the defaultAssemblyLoadContext
. The solution is to makeAIShell
depend on PowerShell SDK so that the PowerShell dependencies are moved to the application level and are loaded into the defaultAssemblyLoadContext
. With that, any agent can use PowerShell without needing to pull PowerShell dependency assemblies into the agent's folder.System.Management.Automation.dll
is located, AKA$PSHOME
.AgentAssemblyLoadContext
to make it able to load assemblies and native libs from theruntimes
folder under the agent folder.<xxx>
form, but the character>
is a special operator in PowerShell and hence causing parsing issue.PowerShell
instances (3 by default) in the background as early as theDataRetriever
type gets initialized, so that when we need aPowerShell
instance to perform parameter injection work for a AzPS command, it's likely one will be already available.PairPlaceholdersForPSCode
is for looking for the command and parameter for a given placeholder for AzPS script. Sometimes the AzPS script first assigns all placeholder values to variables and then use those variables as arguments in commands. The method handles that case too.GetArgValuesForAzPS
is for retrieving argument values for the given command and parameter. It depends on argument completion support in Azure PowerShell.Pending issue
I ran into an issue with the completer attribute
LocationCompleterAttribute
in Azure PowerShell:LocationCompleterAttribute
used for many-Location
parameters always time out because it takes ~10 seconds for the first call to get back results.The issue has been reported to @wyunchi-ms, who is working on improving its performance and hopefully will have it fixed in the next Az PowerShell release.
Follow-up tasks
DataRetriever
and other types to cover the scenarios we support.Tracked by #342