FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Get-Help does not work in JEA sessions by chunqingchen · Pull Request #2988 · PowerShell/PowerShell · GitHub

/ PowerShell Public

Get-Help does not work in JEA sessions - #2988

Merged
Mike Richmond (mirichmo) merged 2 commits into
PowerShell:masterfrom
chunqingchen:bugfix2
Mar 23, 2017
ConversationCommits (2)ChecksFiles changed
Merged

Get-Help does not work in JEA sessions#2988
Mike Richmond (mirichmo) merged 2 commits into
PowerShell:masterfrom
chunqingchen:bugfix2

Conversation

Chunqing Chen (chunqingchen) commented Jan 10, 2017 •
edited by joeyaiello
Loading

Copy link
Copy Markdown
Contributor

Repro:

New-PSSessionConfigurationFile -Path .\basicRRS.pssc -SessionType RestrictedRemoteServer -RunAsVirtualAccount
Register-PSSessionConfiguration -Name 'JEARepro' -Path .\basicRRS.pssc
Enter-PSSession . -ConfigurationName JEARepro
Get-Help Select-Object

Expected behavior:
Help content is returned for the "Select-Object" cmdlet

Actual behavior:

Cannot find path '' because it does not exist.
    + CategoryInfo          : ObjectNotFound: (:) [Get-Help], ItemNotFoundException
    + FullyQualifiedErrorId : PathNotFound,Microsoft.PowerShell.Commands.GetHelpCommand

PowerShellTeam added the Review - Needed The PR is being reviewed label Jan 10, 2017

Chunqing Chen (chunqingchen) commented Jan 11, 2017

Copy link
Copy Markdown
Contributor Author

Paul Higinbotham (@PaulHigin) :)

Paul Higinbotham (PaulHigin) commented Jan 11, 2017

Copy link
Copy Markdown
Contributor

Can you please respond to me comment above?

Steve Lee (SteveL-MSFT) commented Jan 25, 2017

Copy link
Copy Markdown
Member

Please add a test

Mike Richmond (mirichmo) commented Feb 17, 2017

Copy link
Copy Markdown
Member

Chunqing Chen (@chunqingchen) Please update this PR with tests per Steve Lee (@SteveL-MSFT) 's request

Dongbo Wang (daxian-dbw) commented Feb 17, 2017

Copy link
Copy Markdown
Member

Chunqing Chen (@chunqingchen) Please use a meaningful title and description for the PR. The current title and description is suitable for an issue but not for a PR.
The guidelines are kept in CONTRIBUTING.md and I quote here:

Add a meaningful title of the PR describing what change you want to check in. Don't simply put: "Fixes issue #5". A better example is: "Add Ensure parameter to New-Item cmdlet", with "Fixes #5" in the PR's body.
When you create a pull request, including a summary of what's included in your changes

Ilya (iSazonov) reviewed Mar 15, 2017
View reviewed changes
Comment thread test/powershell/engine/Remoting/RemoteSession.Basic.Tests.ps1 Outdated

Ilya (iSazonov) Mar 15, 2017

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Please use $helpContent | Should Not Be $null

Chunqing Chen (chunqingchen) Mar 15, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

corrected

Comment thread test/powershell/engine/Remoting/RemoteSession.Basic.Tests.ps1 Outdated

Ilya (iSazonov) Mar 15, 2017

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Please add new line.

Comment thread test/powershell/engine/Remoting/RemoteSession.Basic.Tests.ps1 Outdated

Ilya (iSazonov) Mar 15, 2017

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Please make the title more informative for the test.
Sample - "Get-Help in JEA sessions".

Chunqing Chen (chunqingchen) Mar 15, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

the test suite contains different bug fixes for JEA sessions. The information is contained in each test itself. thank you.

Ilya (iSazonov) Mar 16, 2017

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I believe that we should not mention "bug fixes" in the title and it is better to make comments in It blocks with a link to the bug number below.

# Fix #1234567890

Comment thread test/powershell/engine/Remoting/RemoteSession.Basic.Tests.ps1 Outdated

Ilya (iSazonov) Mar 15, 2017

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Please use correct case and replace -force with -Force.

Chunqing Chen (chunqingchen) Mar 15, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

corrected

Chunqing Chen (chunqingchen) commented Mar 15, 2017

Copy link
Copy Markdown
Contributor Author

Ilya (@iSazonov) your comment has been resolved. thank you

Paul Higinbotham (PaulHigin) reviewed Mar 15, 2017
View reviewed changes
Comment thread src/System.Management.Automation/help/ProviderContext.cs Outdated

Paul Higinbotham (PaulHigin) Jan 10, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

What is the user experience with this change? With this change does help still appear for cmdlets that are available in the JEA session? Please always include a description of the change along rationale for the change and any modification in experience.

With this change we now no longer throw a PathNotFound exception. How does this affect non-JEA sessions when the path cannot be resolved? Is it Ok to suppress the exception in this case?

Comment thread src/System.Management.Automation/help/ProviderContext.cs Outdated

Paul Higinbotham (PaulHigin) Mar 15, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Strange but my previous comment no longer appears. Anyway my concern with this change is that previously an exception was thrown if the resolvedProviderPath is null, but now it continues. This feels like a breaking change since it now can return incomplete help for non-JEA sessions. I am not sure if that is a big deal but maybe it would be possible to detect JEA session before skipping the error.

Ilya (iSazonov) commented Mar 18, 2017

Copy link
Copy Markdown
Collaborator

Test LGTM.

Paul Higinbotham (PaulHigin) commented Mar 20, 2017

Copy link
Copy Markdown
Contributor

LGTM

Chunqing Chen (chunqingchen) commented Mar 21, 2017

Copy link
Copy Markdown
Contributor Author

Dongbo Wang (@daxian-dbw) Hi, dongbo, can you help to merge? :)

Dongbo Wang (daxian-dbw) commented Mar 21, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Chunqing Chen (@chunqingchen) It looks new commits were pushed after the sign-off from Ilya (@iSazonov) and Paul Higinbotham (@PaulHigin). Please work with Mike Richmond (@mirichmo) to see if further review is needed.

Chunqing Chen (chunqingchen) commented Mar 21, 2017

Copy link
Copy Markdown
Contributor Author

Mike Richmond (@mirichmo) Paul Higinbotham (@PaulHigin) The additional change I made is set the test case as pending because our current test harness is not supporting Register-PSSessionConfiguration cmdlet and needs further work around. however it won't blocking the product fix. Paul, would you please confirm if you are good with this?

Mike Richmond (mirichmo) commented Mar 22, 2017

Copy link
Copy Markdown
Member

Paul Higinbotham (@PaulHigin) Please take a look and provide an updated sign off since there were updates since the original sign off.

Paul Higinbotham (PaulHigin) reviewed Mar 22, 2017
View reviewed changes
Comment thread src/System.Management.Automation/help/ProviderContext.cs Outdated

Paul Higinbotham (PaulHigin) Mar 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I just checked and it turns out that ExecutionContext.InitialSessionState property can be null. Please add a check for this.

Paul Higinbotham (PaulHigin) suggested changes Mar 22, 2017
View reviewed changes

Paul Higinbotham (PaulHigin) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Please add check for _executionContext.InitialSessionState == null

Chunqing Chen (chunqingchen) commented Mar 23, 2017

Copy link
Copy Markdown
Contributor Author

Paul Higinbotham (@PaulHigin) the comment has been resolved

Paul Higinbotham (PaulHigin) approved these changes Mar 23, 2017
View reviewed changes

Paul Higinbotham (PaulHigin) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

Mike Richmond (mirichmo) commented Mar 23, 2017

Copy link
Copy Markdown
Member

Chunqing Chen (@chunqingchen) This PR has been approved, but has merge conflicts. Please resolve them and push an update. I'll merge after that.

Mike Richmond (mirichmo) merged commit 1393f45 into PowerShell:master Mar 23, 2017
Ilya (iSazonov) removed the Review - Needed The PR is being reviewed label Mar 27, 2017
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj/PowerShell that referenced this pull request Aug 6, 2026
Get-Help does not work in JEA sessions (PowerShell#2988)
0d3f85f
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Ilya (iSazonov) iSazonov left review comments
+1 more reviewer
Paul Higinbotham (PaulHigin) PaulHigin approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

Mike Richmond (mirichmo)

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL