Skip to content

Conversation

@KirkMunro
Copy link
Contributor

@KirkMunro KirkMunro commented Oct 17, 2018

PR Summary

It is much easier to deal with asynchronous invocation of PowerShell using async and await, but the S.M.A.PowerShell class only exposes BeginInvoke and EndInvoke methods for async support. This PR adds 5 InvokeAsync overloads (one for each BeginInvoke method) to S.M.A.PowerShell to facilitate async invocation of PowerShell commands from .NET.

PR Checklist

@KirkMunro
Copy link
Contributor Author

Note that I have not added unit tests yet for this PR. I looked to see where BeginInvoke and EndInvoke were tested, and they are included as part of a Pester test; however, PowerShell does not support async/await, so Pester does not appear to be an appropriate test framework for these changes. Please identify how these new methods should be automatically tested. I haven't added C# unit tests to this repo yet, so I'm not sure how I should proceed. Thank you.

@iSazonov iSazonov added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Oct 17, 2018
@iSazonov
Copy link
Collaborator

@KirkMunro Have you any gists with demo? It would help to document the enhancement.

@KirkMunro
Copy link
Contributor Author

I don't have examples showing how complicated this can be using BeginInvoke/EndInvoke, because I just went straight to defining these locally in projects where I have used async PowerShell invocation since they are just so much simpler, so I don't have a before/after example to show for comparison. Also, the code is so short, I'll just share it here instead of in a gist. Here's a snippet showing what use of this looks like when you want async invocation of PowerShell from a .NET assembly with these additional methods in place:

using (var ps = PowerShell.Create())
{
    ps.AddCommand("Connect-AzureRmAccount")
      .AddParameter("ServicePrincipal")
      .AddParameter("TenantId", tenantName)
      .AddParameter("Subscription", subscriptionId)
      .AddParameter("Credential", credential);

    var results = await ps.InvokeAsync();
    // Process results
}

@adityapatwardhan
Copy link
Member

@SteveL-MSFT
Copy link
Member

@PowerShell/powershell-committee agree that we should expose a task model for async PowerShell, leaving implementation details to PR review

@SteveL-MSFT SteveL-MSFT added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Oct 24, 2018
@daxian-dbw
Copy link
Member

@BrucePay Can you please review the signature of the proposed task-based async APIs?

In committee's discussion on this, there are proposals that we might want to re-implement the async invocation in the task-based model instead of wrapping the Begin/End Invoke. But as long as the API signatures are reviewed and approved, I guess we can go with wrapping the existing async APIs for now, and replace the implementation of those task-based APIs at later time, if we want to.

@KirkMunro
Copy link
Contributor Author

Regarding c# unit tests, I didn't get to them before going on vacation this week, so I'll come back to this PR next week and catch up.

@iSazonov
Copy link
Collaborator

iSazonov commented Nov 5, 2018

@powershell-maintainers The test/csharp folder is targeted as Linux-only tests. Suggestion is to put new tests in test/xUnit/System.Management.Automation. Sample https://github.com/iSazonov/PowerShell/tree/add-unicode3/test/xUnit/System.Management.Automation

@KirkMunro
Copy link
Contributor Author

@iSazonov I'll move the tests over. Thanks for letting me know the correct path for those tests.

@KirkMunro
Copy link
Contributor Author

Wait. @iSazonov, that path doesn't exist in master, so this is a new process then? How are tests under that path executed locally for verification?

@iSazonov
Copy link
Collaborator

iSazonov commented Nov 5, 2018

@KirkMunro We haven't such tests at all. And my question is for MSFT team where we should place these tests. I point my new unmerged code which have engine API xUnit tests too. I look how CoreFX places API tests and do the same.

@KirkMunro
Copy link
Contributor Author

@iSazonov Ok, thanks. I'll wait until the other maintainers discuss with you what should be done with these then, and follow the process accordingly.

@stale
Copy link

stale bot commented Dec 15, 2018

This PR has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs within 10 days.
Thank you for your contributions.
Community members are welcome to grab these works.

@stale stale bot added the Stale label Dec 15, 2018
@vexx32
Copy link
Collaborator

vexx32 commented Dec 16, 2018

@KirkMunro Looks like @iSazonov has created a place for the xUnit tests in #8356 😄

@stale stale bot removed the Stale label Dec 16, 2018
@iSazonov
Copy link
Collaborator

Yes, binary tests was moved to new folder. Now all tests run parallel (excluding macOS).

@daxian-dbw daxian-dbw added the CL-Engine Indicates that a PR should be marked as an engine change in the Change Log label Mar 2, 2019
@daxian-dbw daxian-dbw changed the title WIP: Add 5 InvokeAsync overloads to S.M.A.PowerShell Add 5 'InvokeAsync' overloads and 'StopAsync' to the 'PowerShell' type Mar 2, 2019
Copy link
Member

@daxian-dbw daxian-dbw left a comment

Choose a reason for hiding this comment

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

You xUnit tests were failing in CIs. Please fix them. I'm also fine that you remove the xUnit tests for this PR.

@KirkMunro
Copy link
Contributor Author

You xUnit tests were failing in CIs. Please fix them. I'm also fine that you remove the xUnit tests for this PR.

I removed them since they were just duplicates of the Pester tests, and I'm not set up at the moment to easily test C# Unit tests (hence the issues with the CI not working).

@KirkMunro
Copy link
Contributor Author

Pending any further feedback, I think this PR is good to go with enough tests to cover the new Async APIs well enough.

Copy link
Member

@daxian-dbw daxian-dbw left a comment

Choose a reason for hiding this comment

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

LGTM.

@daxian-dbw daxian-dbw merged commit 45aba2a into PowerShell:master Mar 6, 2019
@KirkMunro KirkMunro deleted the powershell-invokeasync-methods branch March 6, 2019 18:10
@iSazonov
Copy link
Collaborator

iSazonov commented Mar 6, 2019

@KirkMunro Thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-Engine Indicates that a PR should be marked as an engine change in the Change Log Committee-Reviewed PS-Committee has reviewed this and made a decision

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants