Skip to content

Get-Help does not work in JEA sessions #2988

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
mirichmo merged 2 commits into from
Mar 23, 2017
Merged

Get-Help does not work in JEA sessions #2988

mirichmo merged 2 commits into from
Mar 23, 2017

Conversation

@chunqingchen
Copy link
Contributor

@chunqingchen chunqingchen commented Jan 10, 2017 •
edited by joeyaiello
Loading

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 PowerShellTeam added the Review - Needed The PR is being reviewed label Jan 10, 2017
@chunqingchen
Copy link
Contributor Author

chunqingchen commented Jan 11, 2017

@PaulHigin :)

@PaulHigin
Copy link
Contributor

PaulHigin commented Jan 11, 2017

Can you please respond to me comment above?

@SteveL-MSFT
Copy link
Member

SteveL-MSFT commented Jan 25, 2017

Please add a test

@mirichmo mirichmo added Review - Waiting on Author and removed Review - Needed The PR is being reviewed labels Feb 17, 2017
@mirichmo
Copy link
Member

mirichmo commented Feb 17, 2017

@chunqingchen Please update this PR with tests per @SteveL-MSFT 's request

@daxian-dbw
Copy link
Member

daxian-dbw commented Feb 17, 2017

@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

iSazonov
iSazonov reviewed Mar 15, 2017
View reviewed changes
test/powershell/engine/Remoting/RemoteSession.Basic.Tests.ps1 Outdated
Copy link
Collaborator

@iSazonov iSazonov Mar 15, 2017

Choose a reason for hiding this comment

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

Please use $helpContent | Should Not Be $null

Copy link
Contributor Author

@chunqingchen chunqingchen Mar 15, 2017

Choose a reason for hiding this comment

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

corrected

test/powershell/engine/Remoting/RemoteSession.Basic.Tests.ps1 Outdated
Copy link
Collaborator

@iSazonov iSazonov Mar 15, 2017

Choose a reason for hiding this comment

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

Please add new line.

test/powershell/engine/Remoting/RemoteSession.Basic.Tests.ps1 Outdated
Copy link
Collaborator

@iSazonov iSazonov Mar 15, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@chunqingchen chunqingchen Mar 15, 2017

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@iSazonov iSazonov Mar 16, 2017

Choose a reason for hiding this comment

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

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

test/powershell/engine/Remoting/RemoteSession.Basic.Tests.ps1 Outdated
Copy link
Collaborator

@iSazonov iSazonov Mar 15, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@chunqingchen chunqingchen Mar 15, 2017

Choose a reason for hiding this comment

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

corrected

@chunqingchen
Copy link
Contributor Author

chunqingchen commented Mar 15, 2017

@iSazonov your comment has been resolved. thank you

PaulHigin
PaulHigin reviewed Mar 15, 2017
View reviewed changes
src/System.Management.Automation/help/ProviderContext.cs Outdated
Copy link
Contributor

@PaulHigin PaulHigin Jan 10, 2017

Choose a reason for hiding this comment

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

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?

src/System.Management.Automation/help/ProviderContext.cs Outdated
Copy link
Contributor

@PaulHigin PaulHigin Mar 15, 2017

Choose a reason for hiding this comment

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

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.

@iSazonov
Copy link
Collaborator

iSazonov commented Mar 18, 2017

Test LGTM.

@PaulHigin
Copy link
Contributor

PaulHigin commented Mar 20, 2017

LGTM

@chunqingchen
Copy link
Contributor Author

chunqingchen commented Mar 21, 2017

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

@daxian-dbw
Copy link
Member

daxian-dbw commented Mar 21, 2017 •
edited
Loading

@chunqingchen It looks new commits were pushed after the sign-off from @iSazonov and @PaulHigin. Please work with @mirichmo to see if further review is needed.

@chunqingchen
Copy link
Contributor Author

chunqingchen commented Mar 21, 2017

@mirichmo @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?

@mirichmo
Copy link
Member

mirichmo commented Mar 22, 2017

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

@mirichmo mirichmo added Review - Needed The PR is being reviewed and removed Review - Waiting on Author labels Mar 22, 2017
PaulHigin
PaulHigin reviewed Mar 22, 2017
View reviewed changes
src/System.Management.Automation/help/ProviderContext.cs Outdated
Copy link
Contributor

@PaulHigin PaulHigin Mar 22, 2017

Choose a reason for hiding this comment

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

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

PaulHigin
PaulHigin suggested changes Mar 22, 2017
View reviewed changes
Copy link
Contributor

@PaulHigin PaulHigin left a comment

Choose a reason for hiding this comment

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

Please add check for _executionContext.InitialSessionState == null

@chunqingchen
Copy link
Contributor Author

chunqingchen commented Mar 23, 2017

@PaulHigin the comment has been resolved

PaulHigin
PaulHigin approved these changes Mar 23, 2017
View reviewed changes
Copy link
Contributor

@PaulHigin PaulHigin left a comment

Choose a reason for hiding this comment

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

LGTM

@mirichmo
Copy link
Member

mirichmo commented Mar 23, 2017

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

@mirichmo mirichmo merged commit 1393f45 into PowerShell:master Mar 23, 2017
@iSazonov iSazonov removed the Review - Needed The PR is being reviewed label Mar 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov iSazonov iSazonov left review comments

+1 more reviewer

@PaulHigin PaulHigin PaulHigin approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@mirichmo mirichmo

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@chunqingchen @PaulHigin @SteveL-MSFT @mirichmo @daxian-dbw @iSazonov @PowerShellTeam @msftclas