GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — WIP: Deprecate workflow-related keywords (workflow, sequence, parallel, and inlinescript) by KirkMunro · Pull Request #10318 · PowerShell/PowerShell

Conversation

@KirkMunro

@KirkMunro Kirk Munro (KirkMunro) commented Aug 8, 2019

Copy link
Copy Markdown
Contributor

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

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
  • This PR is ready to merge and is not Work in Progress.
    • If the PR is work in progress, please add the prefix WIP: or [ WIP ] to the beginning of the title (the WIP bot will keep its status check at Pending while the prefix is present) and remove the prefix when the PR is ready.
  • Breaking changes
    • None
    • OR
    • Experimental feature(s) needed
      • Experimental feature name(s):
  • User-facing changes
    • Not Applicable
    • OR
    • Documentation needed
      • Issue filed:
  • Testing - New and feature
    • N/A or can only be tested interactively
    • OR
    • Make sure you've added a new test if existing tests do not effectively test the code changed
  • Tooling
    • I have considered the user experience from a tooling perspective and don't believe tooling will be impacted.
    • OR
    • I have considered the user experience from a tooling perspective and enumerated concerns in the summary. This may include:
      • Impact on PowerShell Editor Services which is used in the PowerShell extension for VSCode (which runs in a different PS Host).
      • Impact on Completions (both in the console and in editors) - one of PowerShell's most powerful features.
      • Impact on PSScriptAnalyzer (which provides linting & formatting in the editor extensions).
      • Impact on EditorSyntax (which provides syntax highlighting with in VSCode, GitHub, and many other editors).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@KirkMunro
deprecate workflow-related keywords
fc43f54
@KirkMunro
Kirk Munro (KirkMunro) requested review from Paul Higinbotham (PaulHigin) and Dongbo Wang (daxian-dbw) as code owners August 8, 2019 15:05
@KirkMunro
update IsWorkflow flag
0edfc91
PoshChan
PoshChan-Bot (PoshChan) reviewed Aug 8, 2019
View reviewed changes
Comment thread src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
@@ -207,7 +207,7 @@ List<CompletionResult> ExecuteGetCommandCommand(bool useModulePrefix)
}

@PoshChan PoshChan-Bot (PoshChan) Aug 8, 2019

Copy link
Copy Markdown
Collaborator

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.

Kirk Munro (@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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
PoshChan
PoshChan-Bot (PoshChan) reviewed Aug 8, 2019
View reviewed changes
Comment thread src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
@@ -207,7 +207,7 @@ List<CompletionResult> ExecuteGetCommandCommand(bool useModulePrefix)
}

@PoshChan PoshChan-Bot (PoshChan) Aug 8, 2019

Copy link
Copy Markdown
Collaborator

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.

Kirk Munro (@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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
PoshChan
PoshChan-Bot (PoshChan) reviewed Aug 8, 2019
View reviewed changes
Comment thread src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
@@ -207,7 +207,7 @@ List<CompletionResult> ExecuteGetCommandCommand(bool useModulePrefix)
}

@PoshChan PoshChan-Bot (PoshChan) Aug 8, 2019

Copy link
Copy Markdown
Collaborator

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.

Kirk Munro (@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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@KirkMunro

Kirk Munro (KirkMunro) commented Aug 8, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

Steve Lee (@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@KirkMunro Kirk Munro (KirkMunro) mentioned this pull request Aug 8, 2019
Code cleanup: bulk removal of deprecated workflow code #10223
Closed
8 of 15 tasks
@PaulHigin

Paul Higinbotham (PaulHigin) commented Aug 8, 2019

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@KirkMunro

Kirk Munro (KirkMunro) commented Aug 9, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov

Ilya (iSazonov) commented Aug 9, 2019

Copy link
Copy Markdown
Collaborator

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
PaulHigin reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@SteveL-MSFT

Steve Lee (SteveL-MSFT) commented Aug 19, 2019

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@KirkMunro Kirk Munro (KirkMunro) mentioned this pull request Aug 19, 2019
PowerShellGet needs to check compatibility before installing a script or module PowerShell/PowerShellGetv2#528
Open
@KirkMunro

Kirk Munro (KirkMunro) commented Aug 19, 2019

Copy link
Copy Markdown
Contributor Author

Issue PowerShell/PowerShellGet#528 has been logged so that the issue can be fixed in PowerShellGet.
SteveL-MSFT reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@KirkMunro
merge master
c78fa8c
PoshChan
PoshChan-Bot (PoshChan) reviewed Aug 23, 2019
View reviewed changes
Comment thread src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
@@ -207,7 +207,7 @@ List<CompletionResult> ExecuteGetCommandCommand(bool useModulePrefix)
}

@PoshChan PoshChan-Bot (PoshChan) Aug 23, 2019

Copy link
Copy Markdown
Collaborator

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.

Kirk Munro (@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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
PoshChan
PoshChan-Bot (PoshChan) reviewed Aug 23, 2019
View reviewed changes
Comment thread src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
@@ -207,7 +207,7 @@ List<CompletionResult> ExecuteGetCommandCommand(bool useModulePrefix)
}

@PoshChan PoshChan-Bot (PoshChan) Aug 23, 2019

Copy link
Copy Markdown
Collaborator

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.

Kirk Munro (@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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
PoshChan
PoshChan-Bot (PoshChan) reviewed Aug 23, 2019
View reviewed changes
Comment thread src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
@@ -207,7 +207,7 @@ List<CompletionResult> ExecuteGetCommandCommand(bool useModulePrefix)
}

@PoshChan PoshChan-Bot (PoshChan) Aug 23, 2019

Copy link
Copy Markdown
Collaborator

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.

Kirk Munro (@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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@iSazonov

Ilya (iSazonov) commented Aug 24, 2019

Copy link
Copy Markdown
Collaborator

I set WIP until we PowerShellGet issue will be resolved.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov Ilya (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
@KirkMunro
Merge branch 'master' into deprecate-workflow-keywords
668f1ab
PoshChan
PoshChan-Bot (PoshChan) reviewed Oct 23, 2019
View reviewed changes
Comment thread src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
@@ -207,7 +207,7 @@ List<CompletionResult> ExecuteGetCommandCommand(bool useModulePrefix)
}

@PoshChan PoshChan-Bot (PoshChan) Oct 23, 2019

Copy link
Copy Markdown
Collaborator

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.

Kirk Munro (@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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
PoshChan
PoshChan-Bot (PoshChan) reviewed Oct 23, 2019
View reviewed changes
Comment thread src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
@@ -207,7 +207,7 @@ List<CompletionResult> ExecuteGetCommandCommand(bool useModulePrefix)
}

@PoshChan PoshChan-Bot (PoshChan) Oct 23, 2019

Copy link
Copy Markdown
Collaborator

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.

Kirk Munro (@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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
PoshChan
PoshChan-Bot (PoshChan) reviewed Oct 23, 2019
View reviewed changes
Comment thread src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
@@ -207,7 +207,7 @@ List<CompletionResult> ExecuteGetCommandCommand(bool useModulePrefix)
}

@PoshChan PoshChan-Bot (PoshChan) Oct 23, 2019

Copy link
Copy Markdown
Collaborator

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.

Kirk Munro (@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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
PaulHigin
Paul Higinbotham (PaulHigin) approved these changes Nov 7, 2019
View reviewed changes

@PaulHigin Paul Higinbotham (PaulHigin) left a comment

Copy link
Copy Markdown
Contributor

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.

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 Dongbo Wang (@daxian-dbw) to also review them.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@TravisEz13 Travis Plunk (TravisEz13) added this to the 7.1.0-preview.1 milestone Nov 23, 2019
@ghost ghost assigned Aditya Patwardhan (adityapatwardhan) Nov 23, 2019
@adityapatwardhan Aditya Patwardhan (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 ghost added the Stale label Mar 24, 2020
@adityapatwardhan Aditya Patwardhan (adityapatwardhan) closed this Apr 18, 2020
@adityapatwardhan Aditya Patwardhan (adityapatwardhan) reopened this Apr 18, 2020
PoshChan
PoshChan-Bot (PoshChan) reviewed Apr 18, 2020
View reviewed changes
Comment thread src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
@@ -207,7 +207,7 @@ List<CompletionResult> ExecuteGetCommandCommand(bool useModulePrefix)
}

@PoshChan PoshChan-Bot (PoshChan) Apr 18, 2020

Copy link
Copy Markdown
Collaborator

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.

Kirk Munro (@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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
PoshChan
PoshChan-Bot (PoshChan) reviewed Apr 18, 2020
View reviewed changes
Comment thread src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
@@ -207,7 +207,7 @@ List<CompletionResult> ExecuteGetCommandCommand(bool useModulePrefix)
}

@PoshChan PoshChan-Bot (PoshChan) Apr 18, 2020

Copy link
Copy Markdown
Collaborator

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.

Kirk Munro (@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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
PoshChan
PoshChan-Bot (PoshChan) reviewed Apr 18, 2020
View reviewed changes
Comment thread src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
@@ -207,7 +207,7 @@ List<CompletionResult> ExecuteGetCommandCommand(bool useModulePrefix)
}

@PoshChan PoshChan-Bot (PoshChan) Apr 18, 2020

Copy link
Copy Markdown
Collaborator

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.

Kirk Munro (@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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@TravisEz13 Travis Plunk (TravisEz13) modified the milestones: 7.1.0-preview.2, 7.1.0-preview.3 Apr 24, 2020
@adityapatwardhan Aditya Patwardhan (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
@adityapatwardhan

Aditya Patwardhan (adityapatwardhan) commented Apr 24, 2020

Copy link
Copy Markdown
Member

Kirk Munro (@KirkMunro) Please fix CI issues and remove WIP from title when ready.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@TravisEz13 Travis Plunk (TravisEz13) modified the milestones: 7.1.0-preview.3, 7.1.0-preview.4 May 14, 2020
@TravisEz13 Travis Plunk (TravisEz13) removed this from the 7.1.0-preview.4 milestone May 22, 2020
@ghost ghost added the Stale label Jun 6, 2020
@ghost

Deleted user (ghost) commented Jun 6, 2020

Copy link
Copy Markdown

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@KirkMunro

Kirk Munro (KirkMunro) commented Jun 13, 2020

Copy link
Copy Markdown
Contributor Author

Aditya Patwardhan (@adityapatwardhan) Here are the problems with this PR:

  1. 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???
  2. 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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ghost 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 ghost added the Review - Needed The PR is being reviewed label Jun 20, 2020
@ghost

Deleted user (ghost) commented Jun 20, 2020

Copy link
Copy Markdown

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@adityapatwardhan

Aditya Patwardhan (adityapatwardhan) commented Jun 29, 2020

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ghost ghost removed the Review - Needed The PR is being reviewed label Jun 29, 2020
@alerickson

alerickson commented Jun 29, 2020

Copy link
Copy Markdown
Member

Aditya Patwardhan (@adityapatwardhan) Kirk Munro (@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ghost ghost added the Review - Needed The PR is being reviewed label Jul 7, 2020
@ghost

Deleted user (ghost) commented Jul 7, 2020

Copy link
Copy Markdown

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@adityapatwardhan

Aditya Patwardhan (adityapatwardhan) commented Jul 21, 2020

Copy link
Copy Markdown
Member

Kirk Munro (@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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ghost ghost removed the Review - Needed The PR is being reviewed label Jul 21, 2020
@ghost ghost added the Review - Needed The PR is being reviewed label Jul 28, 2020
@ghost

Deleted user (ghost) commented Jul 28, 2020

Copy link
Copy Markdown

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@KirkMunro

Kirk Munro (KirkMunro) commented Apr 24, 2021

Copy link
Copy Markdown
Contributor Author

Closing. See #10238 (comment).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@KirkMunro Kirk Munro (KirkMunro) closed this Apr 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@daxian-dbw Dongbo Wang (daxian-dbw) Awaiting requested review from daxian-dbw daxian-dbw is a code owner
2 more reviewers
@PoshChan PoshChan-Bot (PoshChan) PoshChan left review comments
@PaulHigin Paul Higinbotham (PaulHigin) PaulHigin approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@adityapatwardhan Aditya Patwardhan (adityapatwardhan)

Labels

Review - Needed The PR is being reviewed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

9 participants

@KirkMunro @PaulHigin @iSazonov @SteveL-MSFT @adityapatwardhan @PoshChan @alerickson @daxian-dbw @TravisEz13
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.