PowerShell / PowerShell

PowerShell for every system!
https://microsoft.com/PowerShell
MIT License
44.59k stars 7.21k forks source link

Can we remove workflow code from PowerShell Core? #9570

Closed KirkMunro closed 4 years ago

KirkMunro commented 5 years ago

ActionPreference.Suspend is not supported in PowerShell Core (it's only used for workflows, which are not in PowerShell Core). Today it just adds confusion to the code with checks to ensure that value isn't used where there doesn't need to be any. The checks should also be performed by a PSSA rule rather than in source code (similar to ActionPreference.Ignore, as was decided in #4348). Do we need to keep ActionPreference.Suspend checks around at this point or can we just remove the half-dozen checks related to it from the source code?

Further, if we remove these checks (for the same reason we removed the checks for ActionPreference.Ignore), do we need the ActionPreference.Suspend enumeration value kept around at all?

@SteveL-MSFT: For committee review.

[Update] As per the committee review, all workflow code should be removed. Here is the list of items requiring removal:

iSazonov commented 5 years ago

If this does not work in any case it can be removed. We could use our suggestion system to inform users.

SteveL-MSFT commented 5 years ago

@PowerShell/powershell-committee reviewed this and agree but should broaden this to reviewing all the code and remove all PSWorkflow code

iSazonov commented 5 years ago

There is a lot of such code. Maybe step-by-step will be suitable.

Should we be worried about remoting?

KirkMunro commented 5 years ago

Is there really a lot? Here is the list of things that need to be removed that I am aware of:

That's not that long of a list. In terms of doing it piecemeal or all at once, I removed all references to ActionPreference.Suspend as well as the enumeration value itself as part of PR #8205. It made great sense to do so, since a new ActionPreference enumeration value was being added in that PR (removing it now means the enumeration can be defined without specifically assigned values/gaps caused by removing it later); however, I don't feel removing the rest all at once is too daunting of a task.

iSazonov commented 5 years ago

@KirkMunro Thanks for the review! You could move your check list in the issue description and rename header.

I expected that almost everything was already removed there since we had already cleaned the code several times. Therefore, I was a little surprised that there is still relatively a lot of unnecessary code.

I believe we will make the cleanup step-by-step. Perhaps we will need some discussions because there is public APIs that I already catched in #9618.

KirkMunro commented 5 years ago

@iSazonov Good idea, and done.

lzybkr commented 5 years ago

There are good arguments for not removing keywords:

msftrncs commented 5 years ago

I also do not think that the keywords or parameter names should be moved entirely, and they are all already flagged as errors which is the right thing to do, in my opinion. The only downside to this is that means the workflow keyword cannot be used for a function name without using & (call).

SteveL-MSFT commented 5 years ago

Part of the reasoning by @PowerShell/powershell-committee to remove is to have the errors at parse time/compile time (for C# code) vs at runtime. There is no plan to ever bring back workflow. Instead, Jeffrey has a proposal for workflow-like state management that I'll publish a RFC for eventually.

KirkMunro commented 5 years ago

There is a public method on the Debugger class called GetWorkflowDebuggerFunctions that is not used internally and seems intended only for workflow. Given that is the case, can this (and the related const string function definitions) be removed?

SteveL-MSFT commented 5 years ago

@KirkMunro we've removed other workflow specific code that impacts public API, so now would be the time to remove it

KirkMunro commented 5 years ago

@SteveL-MSFT PowerShellGet internally uses the IsWorkflow property of FunctionDefinitionAst objects. How do you want that handled? I have that property removed from PR #10223 at the moment, which causes some Pester tests to fail because that property does not exist.

Futher, since PowerShellGet is managed independently of PowerShell itself, why are there PowerShellGet tests that use whatever latest version a user has installed in the PowerShell test suite?

iSazonov commented 5 years ago

It is not time critical to leave the property as dummy, then cleanup PowerShellGet, and then remove the property from ast.

daxian-dbw commented 5 years ago

I'm concerned about removing the workflow keyword. An error message like Workflow is not supported in PowerShell 6+ is better than The term 'workflow' is not recognized as the name of a cmdlet .... /cc @SteveL-MSFT

iSazonov commented 5 years ago

I'm concerned about removing the workflow keyword.

@daxian-dbw Please look discussion in #10223

ghost commented 4 years ago

This issue has been marked as fixed and has not had any activity for 1 day. It has been closed for housekeeping purposes.

mavaddat commented 1 year ago

It would be cool if PowerShell Core provided modularity on the workflow engine choice so that endusers could opt to choose their workflow adapter assembly. I have floated this idea in the UiPath/CoreWF discussion.

image