Skip to content

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

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

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

Conversation

@chunqingchen

@chunqingchen Chunqing Chen (chunqingchen) commented Jan 10, 2017

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

@chunqingchen

Copy link
Copy Markdown
Contributor Author

@PaulHigin

Copy link
Copy Markdown
Contributor

Can you please respond to me comment above?

@SteveL-MSFT

Copy link
Copy Markdown
Member

Please add a test

@mirichmo

Copy link
Copy Markdown
Member

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

@daxian-dbw

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

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

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

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.

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".

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.

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

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.

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

Copy link
Copy Markdown
Contributor Author

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

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?

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

Copy link
Copy Markdown
Collaborator

Test LGTM.

@PaulHigin

Copy link
Copy Markdown
Contributor

LGTM

@chunqingchen

Copy link
Copy Markdown
Contributor Author

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

@daxian-dbw

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

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.

@chunqingchen

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?

@mirichmo

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.

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.

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

Copy link
Copy Markdown
Contributor Author

Paul Higinbotham (@PaulHigin) the comment has been resolved

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

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.

@mirichmo
Mike Richmond (mirichmo) merged commit 1393f45 into PowerShell:master Mar 23, 2017
@iSazonov 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants