WIP: Deprecate workflow-related keywords (workflow, sequence, parallel, and inlinescript) #10318

KirkMunro · 2019-08-08T15:05:29Z

PR Summary

Cleanup of workflow code that has been deprecated. In particular:

deprecation of workflow keyword
deprecation of parallel keyword (used for parallel activities in workflow)
deprecation of sequence keyword (used for sequential activities in workflow)
deprecation of inlinescript activity (used for activities outside of workflow)
removal of some related methods
removal of deprecated resource strings
updates to non-deprecated resource strings
removal of the use of "workflow" in many comments

The keywords in question have been preserved, but will now generate parser errors because they are deprecated.

PR Context

See issue #9570.

PR Checklist

PoshChan · 2019-08-08T15:23:25Z

src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs


                return completionResults;
            }
        }


@KirkMunro, your last commit had 2 failures in PowerShell-CI-macos
PowerShellGet - Script tests (Admin).Should install a script correctly to the required location with AllUsers scope

Expected a value, but got $null or empty. at <ScriptBlock>, /Users/vsts/agent/2.155.1/work/1/s/test/powershell/Modules/PowerShellGet/PowerShellGet.Tests.ps1: line 234 234: $installedScriptInfo | Should -Not -BeNullOrEmpty

PowerShellGet - Script tests.Should install a script correctly to the required location with default CurrentUser scope

Expected a value, but got $null or empty. at <ScriptBlock>, /Users/vsts/agent/2.155.1/work/1/s/test/powershell/Modules/PowerShellGet/PowerShellGet.Tests.ps1: line 207 207: $installedScriptInfo | Should -Not -BeNullOrEmpty

PoshChan · 2019-08-08T15:25:00Z

src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs


                return completionResults;
            }
        }


@KirkMunro, your last commit had 2 failures in PowerShell-CI-windows
PowerShellGet - Script tests (Admin).Should install a script correctly to the required location with AllUsers scope

Expected a value, but got $null or empty. at <ScriptBlock>, D:\a\1\s\test\powershell\Modules\PowerShellGet\PowerShellGet.Tests.ps1: line 234 234: $installedScriptInfo | Should -Not -BeNullOrEmpty

PowerShellGet - Script tests.Should install a script correctly to the required location with default CurrentUser scope

Expected a value, but got $null or empty. at <ScriptBlock>, D:\a\1\s\test\powershell\Modules\PowerShellGet\PowerShellGet.Tests.ps1: line 207 207: $installedScriptInfo | Should -Not -BeNullOrEmpty

PoshChan · 2019-08-08T15:25:36Z

src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs


                return completionResults;
            }
        }


@KirkMunro, your last commit had 2 failures in PowerShell-CI-linux
PowerShellGet - Script tests (Admin).Should install a script correctly to the required location with AllUsers scope

Expected a value, but got $null or empty. at <ScriptBlock>, /home/vsts/work/1/s/test/powershell/Modules/PowerShellGet/PowerShellGet.Tests.ps1: line 234 234: $installedScriptInfo | Should -Not -BeNullOrEmpty

PowerShellGet - Script tests.Should install a script correctly to the required location with default CurrentUser scope

Expected a value, but got $null or empty. at <ScriptBlock>, /home/vsts/work/1/s/test/powershell/Modules/PowerShellGet/PowerShellGet.Tests.ps1: line 207 207: $installedScriptInfo | Should -Not -BeNullOrEmpty

KirkMunro · 2019-08-08T19:52:00Z

@SteveL-MSFT The two Pester errors that occur on each platform happen because Fabrikam-ServerScript in www.poshtestgallery.com defines a workflow. Since workflow does not parse with this PR in place, and since PowerShellGet wasn't designed with deprecated features that generate parse errors in mind, there's a chicken-and-egg problem with these tests.

Since I can use Find-Script in the same PowerShellGet module in a build with this PR to find the script and pull script metadata, which includes workflows that are in the script, we could update PowerShellGet to check for the presence of workflow as part of Install-Script (the information is obviously retrievable, and if workflow is found), and prevent the installation from continuing since it is not supported on 7.0+. We could then update the failing tests to verify that scenario fails properly, and to test that installing a different script that does not have workflow works. This will allow the Pester tests to pass.

Longer term, we could also update PowerShellGet to recognize parser errors about deprecated keywords. The parser error that is being generated by the parser in this PR is very specific, and clearly identifies when a keyword is deprecated. The keyword token also has a flag indicating that it is deprecated. In the case where a deprecated keyword is found, PowerShellGet could simply ignore that error since it shouldn't care.

You can see both the descriptive error id and the keyword Deprecated flag in the output below:

PS C:\> $tokens = $null
>> $errors = $null
>> [System.Management.Automation.Language.Parser]::ParseInput(
>>     'workflow foo {''bar''}; Get-Date',
>>      [ref]$tokens,
>>      [ref]$errors)

Attributes         : {}
UsingStatements    : {}
ParamBlock         :
BeginBlock         :
ProcessBlock       :
EndBlock           : workflow foo {'bar'}; Get-Date
DynamicParamBlock  :
ScriptRequirements :
Extent             : workflow foo {'bar'}; Get-Date
Parent             :


PS C:\> $errors | fl *

Extent          : workflow
ErrorId         : DeprecatedKeywordNotAllowed
Message         : The 'workflow' keyword has been deprecated and is no longer supported.
IncompleteInput : False


PS C:\> $tokens

Text       : workflow
TokenFlags : Keyword, Deprecated
Kind       : Workflow
HasError   : False
Extent     : workflow

Text       : foo
TokenFlags : CommandName
Kind       : Identifier
HasError   : False
Extent     : foo

Text       : {
TokenFlags : ParseModeInvariant
Kind       : LCurly
HasError   : False
Extent     : {

Value      : bar
Text       : 'bar'
TokenFlags : ParseModeInvariant
Kind       : StringLiteral
HasError   : False
Extent     : 'bar'

Text       : }
TokenFlags : ParseModeInvariant
Kind       : RCurly
HasError   : False
Extent     : }

Text       : ;
TokenFlags : ParseModeInvariant
Kind       : Semi
HasError   : False
Extent     : ;

Value      : Get-Date
Text       : Get-Date
TokenFlags : CommandName
Kind       : Generic
HasError   : False
Extent     : Get-Date

Text       :
TokenFlags : ParseModeInvariant
Kind       : EndOfInput
HasError   : False
Extent     :


PS C:\>

Please let me know how you would like to proceed.

PaulHigin · 2019-08-08T23:22:04Z

/cc @SteveL-MSFT
I am not sure if it is worth the effort. Perhaps we should continue with other workflow cleanup.

KirkMunro · 2019-08-09T02:05:03Z

If we do want these keywords deprecated, the simple solution for PowerShellGet is to check if a script/module has workflow in it before installing it, and prevent that installation from happening if that is the case in PowerShell 7+. That's logical, necessary, and the right thing to do. Users can still save the script, which does not do any parsing checks, and that is the most they would want to do in a version of PowerShell that doesn't support workflow anyway. They shouldn't be able to install a script that is incompatible even with -Force (why would I be able to install something I can't use -- installers prevent such things, and rightly so). Since that change should probably happen regardless, and since that change fixes these issues, we could move forward with little effort with that change plus splitting the tests in question into two, so that they test for success and failure when it comes to workflow.

iSazonov · 2019-08-09T11:52:33Z

Today the cleanup is not critical and we could do not break PowerShellGet in the PR. Although we need an understanding of how to proceed in future

SteveL-MSFT · 2019-08-19T16:25:04Z

For now, perhaps we should have an issue opened in PowerShellGet repo to produce an error if a script is incompatible. For this PR, we can wait on this since it's desirable, but not critical.

KirkMunro · 2019-08-19T16:54:27Z

Issue PowerShell/PowerShellGet#528 has been logged so that the issue can be fixed in PowerShellGet.

PoshChan · 2019-08-23T18:39:34Z

src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs


                return completionResults;
            }
        }


@KirkMunro, your last commit had 2 failures in PowerShell-CI-macos
PowerShellGet - Script tests (Admin).Should install a script correctly to the required location with AllUsers scope

Expected a value, but got $null or empty. at <ScriptBlock>, /Users/vsts/agent/2.155.1/work/1/s/test/powershell/Modules/PowerShellGet/PowerShellGet.Tests.ps1: line 234 234: $installedScriptInfo | Should -Not -BeNullOrEmpty

PowerShellGet - Script tests.Should install a script correctly to the required location with default CurrentUser scope

Expected a value, but got $null or empty. at <ScriptBlock>, /Users/vsts/agent/2.155.1/work/1/s/test/powershell/Modules/PowerShellGet/PowerShellGet.Tests.ps1: line 207 207: $installedScriptInfo | Should -Not -BeNullOrEmpty

PoshChan · 2019-08-23T18:42:34Z

src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs


                return completionResults;
            }
        }


@KirkMunro, your last commit had 2 failures in PowerShell-CI-linux
PowerShellGet - Script tests (Admin).Should install a script correctly to the required location with AllUsers scope

Expected a value, but got $null or empty. at <ScriptBlock>, /home/vsts/work/1/s/test/powershell/Modules/PowerShellGet/PowerShellGet.Tests.ps1: line 234 234: $installedScriptInfo | Should -Not -BeNullOrEmpty

PowerShellGet - Script tests.Should install a script correctly to the required location with default CurrentUser scope

Expected a value, but got $null or empty. at <ScriptBlock>, /home/vsts/work/1/s/test/powershell/Modules/PowerShellGet/PowerShellGet.Tests.ps1: line 207 207: $installedScriptInfo | Should -Not -BeNullOrEmpty

PoshChan · 2019-08-23T18:42:35Z

src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs


                return completionResults;
            }
        }


@KirkMunro, your last commit had 2 failures in PowerShell-CI-windows
PowerShellGet - Script tests (Admin).Should install a script correctly to the required location with AllUsers scope

Expected a value, but got $null or empty. at <ScriptBlock>, D:\a\1\s\test\powershell\Modules\PowerShellGet\PowerShellGet.Tests.ps1: line 234 234: $installedScriptInfo | Should -Not -BeNullOrEmpty

PowerShellGet - Script tests.Should install a script correctly to the required location with default CurrentUser scope

Expected a value, but got $null or empty. at <ScriptBlock>, D:\a\1\s\test\powershell\Modules\PowerShellGet\PowerShellGet.Tests.ps1: line 207 207: $installedScriptInfo | Should -Not -BeNullOrEmpty

iSazonov · 2019-08-24T15:36:27Z

I set WIP until we PowerShellGet issue will be resolved.

PoshChan · 2019-10-23T20:02:39Z

src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs


                return completionResults;
            }
        }


@KirkMunro, your last commit had 2 failures in PowerShell-CI-linux
PowerShellGet - Script tests (Admin).Should install a script correctly to the required location with AllUsers scope

Expected a value, but got $null or empty. at <ScriptBlock>, /home/vsts/work/1/s/test/powershell/Modules/PowerShellGet/PowerShellGet.Tests.ps1: line 234 234: $installedScriptInfo | Should -Not -BeNullOrEmpty

PowerShellGet - Script tests.Should install a script correctly to the required location with default CurrentUser scope

Expected a value, but got $null or empty. at <ScriptBlock>, /home/vsts/work/1/s/test/powershell/Modules/PowerShellGet/PowerShellGet.Tests.ps1: line 207 207: $installedScriptInfo | Should -Not -BeNullOrEmpty

PoshChan · 2019-10-23T20:03:31Z

src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs


                return completionResults;
            }
        }


@KirkMunro, your last commit had 2 failures in PowerShell-CI-windows
PowerShellGet - Script tests (Admin).Should install a script correctly to the required location with AllUsers scope

Expected a value, but got $null or empty. at <ScriptBlock>, D:\a\1\s\test\powershell\Modules\PowerShellGet\PowerShellGet.Tests.ps1: line 234 234: $installedScriptInfo | Should -Not -BeNullOrEmpty

PowerShellGet - Script tests.Should install a script correctly to the required location with default CurrentUser scope

Expected a value, but got $null or empty. at <ScriptBlock>, D:\a\1\s\test\powershell\Modules\PowerShellGet\PowerShellGet.Tests.ps1: line 207 207: $installedScriptInfo | Should -Not -BeNullOrEmpty

PoshChan · 2019-10-23T20:16:08Z

src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs


                return completionResults;
            }
        }


@KirkMunro, your last commit had 2 failures in PowerShell-CI-macos
PowerShellGet - Script tests (Admin).Should install a script correctly to the required location with AllUsers scope

Expected a value, but got $null or empty. at <ScriptBlock>, /Users/runner/runners/2.158.0/work/1/s/test/powershell/Modules/PowerShellGet/PowerShellGet.Tests.ps1: line 234 234: $installedScriptInfo | Should -Not -BeNullOrEmpty

PowerShellGet - Script tests.Should install a script correctly to the required location with default CurrentUser scope

Expected a value, but got $null or empty. at <ScriptBlock>, /Users/runner/runners/2.158.0/work/1/s/test/powershell/Modules/PowerShellGet/PowerShellGet.Tests.ps1: line 207 207: $installedScriptInfo | Should -Not -BeNullOrEmpty

PaulHigin

These changes look good to me, as long as all tests continue to pass. Since these include a lot of parser/token changes I would like @daxian-dbw to also review them.

PoshChan · 2020-04-18T00:27:21Z

src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs


                return completionResults;
            }
        }


@KirkMunro, your last commit had 2 failures in PowerShell-CI-linux
PowerShellGet - Script tests (Admin).Should install a script correctly to the required location with AllUsers scope

Expected a value, but got $null or empty. at <ScriptBlock>, /home/vsts/work/1/s/test/powershell/Modules/PowerShellGet/PowerShellGet.Tests.ps1: line 234 234: $installedScriptInfo | Should -Not -BeNullOrEmpty

PowerShellGet - Script tests.Should install a script correctly to the required location with default CurrentUser scope

Expected a value, but got $null or empty. at <ScriptBlock>, /home/vsts/work/1/s/test/powershell/Modules/PowerShellGet/PowerShellGet.Tests.ps1: line 207 207: $installedScriptInfo | Should -Not -BeNullOrEmpty

PoshChan · 2020-04-18T00:28:59Z

src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs


                return completionResults;
            }
        }


@KirkMunro, your last commit had 2 failures in PowerShell-CI-macos
PowerShellGet - Script tests (Admin).Should install a script correctly to the required location with AllUsers scope

Expected a value, but got $null or empty. at <ScriptBlock>, /Users/runner/runners/2.165.2/work/1/s/test/powershell/Modules/PowerShellGet/PowerShellGet.Tests.ps1: line 234 234: $installedScriptInfo | Should -Not -BeNullOrEmpty

PowerShellGet - Script tests.Should install a script correctly to the required location with default CurrentUser scope

Expected a value, but got $null or empty. at <ScriptBlock>, /Users/runner/runners/2.165.2/work/1/s/test/powershell/Modules/PowerShellGet/PowerShellGet.Tests.ps1: line 207 207: $installedScriptInfo | Should -Not -BeNullOrEmpty

PoshChan · 2020-04-18T00:29:22Z

src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs


                return completionResults;
            }
        }


@KirkMunro, your last commit had 2 failures in PowerShell-CI-windows
PowerShellGet - Script tests (Admin).Should install a script correctly to the required location with AllUsers scope

Expected a value, but got $null or empty. at <ScriptBlock>, D:\a\1\s\test\powershell\Modules\PowerShellGet\PowerShellGet.Tests.ps1: line 234 234: $installedScriptInfo | Should -Not -BeNullOrEmpty

PowerShellGet - Script tests.Should install a script correctly to the required location with default CurrentUser scope

Expected a value, but got $null or empty. at <ScriptBlock>, D:\a\1\s\test\powershell\Modules\PowerShellGet\PowerShellGet.Tests.ps1: line 207 207: $installedScriptInfo | Should -Not -BeNullOrEmpty

adityapatwardhan · 2020-04-24T18:36:50Z

@KirkMunro Please fix CI issues and remove WIP from title when ready.

ghost · 2020-06-06T20:00:16Z

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 15 days. It will be closed if no further activity occurs within 10 days of this comment.

KirkMunro · 2020-06-13T02:17:09Z

@adityapatwardhan Here are the problems with this PR:

This PR was dependent on another issue (issue PowerShell/PowerShellGet#528). That issue disappeared. I didn't delete it, so how do we find out what happened to that???
Assuming that issue gets recovered (or relogged), this PR can't be finished until that issue is addressed (which obviously won't happen if the issue that was logged was deleted).

That is why this was marked WIP: because it is dependent on an issue that I just discovered is missing. I'd like to know what happened to that issue, because issues should not be deleted.

ghost · 2020-06-20T14:00:15Z

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

adityapatwardhan · 2020-06-29T17:20:49Z

@alerickson - Can you have a look why the issue in PowerShellGet disappeared?

alerickson · 2020-06-29T17:52:12Z

@adityapatwardhan @KirkMunro the name of the PSGet repository for versions below 3 is now called PowerShellGetv2, the issue is still there though: PowerShell/PowerShellGetv2/issues/528. We won't address non-security related issues in that repo, but I'll transfer the issue over to the new repo so we can check before installing scripts in v3.

ghost · 2020-07-07T02:00:25Z

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

adityapatwardhan · 2020-07-21T00:24:08Z

@KirkMunro I guess we will have to wait till the version of PSGet that fixes the issue is included in PowerShell release.
@alerickson Is the fix for the issue going to be part of PSGet v3? or does it not repro in v3 at all?

ghost · 2020-07-28T02:00:19Z

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

KirkMunro · 2021-04-24T13:18:14Z

Closing. See #10238 (comment).

deprecate workflow-related keywords

fc43f54

KirkMunro requested review from PaulHigin and daxian-dbw as code owners August 8, 2019 15:05

update IsWorkflow flag

0edfc91

PoshChan reviewed Aug 8, 2019

View reviewed changes

KirkMunro mentioned this pull request Aug 8, 2019

Code cleanup: bulk removal of deprecated workflow code #10223

Closed

15 tasks

KirkMunro mentioned this pull request Aug 19, 2019

PowerShellGet needs to check compatibility before installing a script or module PowerShell/PowerShellGetv2#528

Open

merge master

c78fa8c

PoshChan reviewed Aug 23, 2019

View reviewed changes

iSazonov changed the title ~~Deprecate workflow-related keywords (workflow, sequence, parallel, and inlinescript)~~ WIP: Deprecate workflow-related keywords (workflow, sequence, parallel, and inlinescript) Aug 24, 2019

Merge branch 'master' into deprecate-workflow-keywords

668f1ab

PoshChan reviewed Oct 23, 2019

View reviewed changes

PaulHigin approved these changes Nov 7, 2019

View reviewed changes

TravisEz13 added this to the 7.1.0-preview.1 milestone Nov 23, 2019

ghost assigned adityapatwardhan Nov 23, 2019

adityapatwardhan added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Feb 20, 2020

ghost added the Stale label Mar 24, 2020

adityapatwardhan closed this Apr 18, 2020

adityapatwardhan reopened this Apr 18, 2020

PoshChan reviewed Apr 18, 2020

View reviewed changes

TravisEz13 modified the milestones: 7.1.0-preview.2, 7.1.0-preview.3 Apr 24, 2020

adityapatwardhan added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Apr 24, 2020

TravisEz13 modified the milestones: 7.1.0-preview.3, 7.1.0-preview.4 May 14, 2020

TravisEz13 removed this from the 7.1.0-preview.4 milestone May 22, 2020

ghost added the Stale label Jun 6, 2020

ghost removed Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept Stale labels Jun 13, 2020

ghost added the Review - Needed The PR is being reviewed label Jun 20, 2020

ghost removed the Review - Needed The PR is being reviewed label Jun 29, 2020

ghost added the Review - Needed The PR is being reviewed label Jul 7, 2020

ghost removed the Review - Needed The PR is being reviewed label Jul 21, 2020

ghost added the Review - Needed The PR is being reviewed label Jul 28, 2020

KirkMunro closed this Apr 24, 2021

WIP: Deprecate workflow-related keywords (workflow, sequence, parallel, and inlinescript) #10318

WIP: Deprecate workflow-related keywords (workflow, sequence, parallel, and inlinescript) #10318

Uh oh!

Conversation

KirkMunro commented Aug 8, 2019

PR Summary

PR Context

PR Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KirkMunro commented Aug 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PaulHigin commented Aug 8, 2019

Uh oh!

KirkMunro commented Aug 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Aug 9, 2019

Uh oh!

SteveL-MSFT commented Aug 19, 2019

Uh oh!

KirkMunro commented Aug 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Aug 24, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PaulHigin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adityapatwardhan commented Apr 24, 2020

Uh oh!

ghost commented Jun 6, 2020

Uh oh!

KirkMunro commented Jun 13, 2020

Uh oh!

ghost commented Jun 20, 2020

Uh oh!

adityapatwardhan commented Jun 29, 2020

Uh oh!

alerickson commented Jun 29, 2020

Uh oh!

ghost commented Jul 7, 2020

Uh oh!

adityapatwardhan commented Jul 21, 2020

Uh oh!

ghost commented Jul 28, 2020

Uh oh!

KirkMunro commented Apr 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

KirkMunro commented Aug 8, 2019 •

edited

Loading

KirkMunro commented Aug 9, 2019 •

edited

Loading