Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Get-Help does not work in JEA sessions#2988

Merged
mirichmo merged 2 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
chunqingchen:bugfix2Copy head branch name to clipboard
Mar 23, 2017
Merged

Get-Help does not work in JEA sessions#2988
mirichmo merged 2 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
chunqingchen:bugfix2Copy head branch name to clipboard

Conversation

@chunqingchen

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

chunqingchen commented Jan 11, 2017

Copy link
Copy Markdown
Contributor Author

@PaulHigin :)

@PaulHigin

PaulHigin commented Jan 11, 2017

Copy link
Copy Markdown
Contributor

Can you please respond to me comment above?

@SteveL-MSFT

SteveL-MSFT commented Jan 25, 2017

Copy link
Copy Markdown
Member

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

mirichmo commented Feb 17, 2017

Copy link
Copy Markdown
Member

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

@daxian-dbw

daxian-dbw commented Feb 17, 2017

Copy link
Copy Markdown
Member

@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
Comment thread test/powershell/engine/Remoting/RemoteSession.Basic.Tests.ps1 Outdated

@iSazonov 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.

Please use $helpContent | Should Not Be $null

@chunqingchen 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.

corrected

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

@iSazonov 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.

Please add new line.

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

@iSazonov 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.

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

@chunqingchen 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.

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

@iSazonov 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.

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

@iSazonov 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.

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

@chunqingchen 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.

corrected

@chunqingchen

chunqingchen commented Mar 15, 2017

Copy link
Copy Markdown
Contributor Author

@iSazonov your comment has been resolved. thank you

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

@PaulHigin 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.

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

@PaulHigin 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.

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

iSazonov commented Mar 18, 2017

Copy link
Copy Markdown
Collaborator

Test LGTM.

@PaulHigin

PaulHigin commented Mar 20, 2017

Copy link
Copy Markdown
Contributor

LGTM

@chunqingchen

chunqingchen commented Mar 21, 2017

Copy link
Copy Markdown
Contributor Author

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

@daxian-dbw

daxian-dbw commented Mar 21, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

@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

chunqingchen commented Mar 21, 2017

Copy link
Copy Markdown
Contributor Author

@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

mirichmo commented Mar 22, 2017

Copy link
Copy Markdown
Member

@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
Comment thread src/System.Management.Automation/help/ProviderContext.cs Outdated

@PaulHigin 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.

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

@PaulHigin 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.

Please add check for _executionContext.InitialSessionState == null

@chunqingchen

chunqingchen commented Mar 23, 2017

Copy link
Copy Markdown
Contributor Author

@PaulHigin the comment has been resolved

PaulHigin
PaulHigin approved these changes Mar 23, 2017
View reviewed changes

@PaulHigin 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.

LGTM

@mirichmo

mirichmo commented Mar 23, 2017

Copy link
Copy Markdown
Member

@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
Morty Proxy This is a proxified and sanitized view of the page, visit original site.