Enable WSManCredSSP cmdlets by SteveL-MSFT · Pull Request #4336 · PowerShell/PowerShell

SteveL-MSFT · 2017-07-24T23:15:14Z

Fixed error message using a null resourcemanager
With most recent CoreFx supporting STA, removed special CoreClr code paths
Added test coverage
Removed some dead commented out code that was never used

Part of #4158

Fix #3353

SteveL-MSFT · 2017-07-25T16:36:39Z

AppVeyor failure waiting on #4323

mirichmo · 2017-07-26T00:59:26Z

src/Microsoft.WSMan.Management/CredSSP.cs

Why were these two checks removed?

line 486 and 890 already does the check

mirichmo · 2017-07-26T01:08:15Z

src/Microsoft.WSMan.Management/WsManHelper.cs

Ca you rename this variable to remove the initial "g"?

mirichmo · 2017-07-26T01:08:30Z

src/Microsoft.WSMan.Management/CredSSP.cs

Why were these two checks removed?

check happens twice

mirichmo · 2017-07-26T01:11:05Z

test/powershell/Modules/Microsoft.WSMan.Management/CredSSP-nonAdmin.Tests.ps1

Does this line work on non-Windows platforms?

Code should not be outside of a Pester block. This should be moved within a BeforeAll

Check out the guidelines on Admin for Windows

Consider using the pattern for skipping tests as well.

will wrap in $IsWindows

will wrap in BeforeAll

this is the opposite of requiring admin, this test requires NOT admin (there isn't a RequireNotAdminOnWindows)

two different test cases here where the first is only applicable when NOT admin, the second one works either way

mirichmo · 2017-07-26T01:17:52Z

test/powershell/Modules/Microsoft.WSMan.Management/CredSSP.Tests.ps1

Why are these tests separated into separate files? We have been grouping tests for each cmdlet into a single file.

I suppose I can combine them. I thought to separate them because at least one test case requires non-admin.

Just tried it combined and start-pspester only ran the tests as elevated so that one test gets skipped. I'll have to have the non-admin tests as separate.

Actually it appears you have explicitly run start-pspester with -unelevated. I'll have to take care of this myself in the script then.

mirichmo · 2017-07-26T01:18:23Z

test/powershell/Modules/Microsoft.WSMan.Management/CredSSP.Tests.ps1

The same comments apply to this file as well

SteveL-MSFT · 2017-07-26T03:33:32Z

src/Microsoft.WSMan.Management/CredSSP.cs

this is redundant from line 486

SteveL-MSFT · 2017-07-26T20:49:51Z

CI failure is due to one of the same failures in the nightly run unrelated to this PR. I'll fix that first.

adityapatwardhan · 2017-07-28T21:07:23Z

test/powershell/Modules/Microsoft.WSMan.Management/CredSSP.Tests.ps1

Please check for en-US before comparing.

adityapatwardhan · 2017-07-28T21:07:50Z

test/powershell/Modules/Microsoft.WSMan.Management/CredSSP.Tests.ps1

Same as above

adityapatwardhan · 2017-07-28T21:08:29Z

test/powershell/Modules/Microsoft.WSMan.Management/CredSSP.Tests.ps1

Same as above.

adityapatwardhan · 2017-07-28T21:08:47Z

test/powershell/Modules/Microsoft.WSMan.Management/CredSSP.Tests.ps1

Same as above

SteveL-MSFT · 2017-08-01T04:39:39Z

@mirichmo @adityapatwardhan feedback addressed

SteveL-MSFT · 2017-08-04T02:26:54Z

@mirichmo can you complete your review?

mirichmo · 2017-08-07T21:41:51Z

test/powershell/Modules/Microsoft.WSMan.Management/CredSSP.Tests.ps1

As a general rule when using this pattern, a test that is skipped in an environment should not execute any code.

Lines 11 through 15 should not be executed if the test is skipped. Putting them in an else {} would solve this problem.

adityapatwardhan · 2017-08-08T00:33:44Z

@mirichmo Can you rebase, it should fix the test failures.

Fixed error message using a null resourcemanager With most recent CoreFx supporting STA, removed special CoreClr code paths Added test coverage Removed some dead commented out code that was never used

merged tests to one file, use runas to have relevant tests run as non-elevated

Get-CredSSP returns text so only run that test if current culture is en-US

adityapatwardhan · 2017-08-08T17:14:39Z

Tests are failing on Linux.

mirichmo · 2017-08-09T16:40:11Z

All the CredSSP tests are now passing in AppVeyor and Travis.

ltamaster · 2017-11-22T20:37:05Z

Hi All,
I was looking to use the WSManCredSSP functionalities.
has this PR enabled in a release?
I checked the v6.0.0-beta.6 because it mentions that it was added, but it didn't work for me.

Thanks
Luis

SteveL-MSFT · 2017-11-22T23:50:06Z

@ltamaster CredSSP is only supported on Windows

iSazonov · 2017-11-23T04:00:05Z

@SteveL-MSFT It seems PowerShell docs haven't "Supported platforms: Windows, Linux, MacOS" in cmdlet's descriptions.
Also we could make the error message more clear: "The term Cmdlet is not supported on the platform or is not recognized ..."

SteveL-MSFT · 2017-11-23T21:20:12Z

@iSazonov can you point me to the docs that say it's supported on Linux/macOS? Don't see that line here

I think the error message is technically correct as that cmdlet doesn't exist on non-Windows. It's a generic error message if a cmdlet is not found, so implying it's not supported may cause confusion particularly for typos.

iSazonov · 2017-11-24T03:39:58Z

@SteveL-MSFT I agree that we can leave the generic message "as is" if we enhance our docs.

can you point me to the docs that say it's supported on Linux/macOS?

It is just my request to enhance our docs - add new section "Supported platforms".

SteveL-MSFT assigned adityapatwardhan Jul 24, 2017

SteveL-MSFT requested a review from mirichmo July 24, 2017 23:15

msftclas added the cla-not-required label Jul 24, 2017

SteveL-MSFT force-pushed the credssp branch from 74ca739 to b049a00 Compare July 24, 2017 23:15

SteveL-MSFT requested a review from adityapatwardhan July 24, 2017 23:33

mirichmo suggested changes Jul 26, 2017

View reviewed changes

SteveL-MSFT force-pushed the credssp branch from b049a00 to f8612e2 Compare July 26, 2017 03:32

SteveL-MSFT commented Jul 26, 2017

View reviewed changes

src/Microsoft.WSMan.Management/CredSSP.cs Outdated

Copy link
Copy Markdown

Member Author

SteveL-MSFT Jul 26, 2017

Choose a reason for hiding this comment

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

this is redundant from line 486

SteveL-MSFT force-pushed the credssp branch 2 times, most recently from 1e9b8af to a06b389 Compare July 26, 2017 16:25

adityapatwardhan requested changes Jul 28, 2017

View reviewed changes

SteveL-MSFT force-pushed the credssp branch 2 times, most recently from 1501d34 to c74be60 Compare August 1, 2017 00:48

adityapatwardhan approved these changes Aug 1, 2017

View reviewed changes

mirichmo suggested changes Aug 7, 2017

View reviewed changes

mirichmo force-pushed the credssp branch from 741fef2 to 0f93089 Compare August 7, 2017 22:56

mirichmo approved these changes Aug 7, 2017

View reviewed changes

SteveL-MSFT and others added 4 commits August 8, 2017 09:23

[Feature]

a6b8347

Fixed error message using a null resourcemanager With most recent CoreFx supporting STA, removed special CoreClr code paths Added test coverage Removed some dead commented out code that was never used

[Feature]

e61c05a

merged tests to one file, use runas to have relevant tests run as non-elevated

[Feature]

efa769f

Get-CredSSP returns text so only run that test if current culture is en-US

[Feature] Update test to fix code exclusion

7d1c523

mirichmo force-pushed the credssp branch from 0f93089 to 7d1c523 Compare August 8, 2017 16:23

[Feature] Fix Skipping on test cases that already had a Skip value

cf9ee3f

adityapatwardhan merged commit eea3885 into PowerShell:master Aug 10, 2017

SteveL-MSFT deleted the credssp branch August 14, 2017 06:25

Conversation

SteveL-MSFT commented Jul 24, 2017

Uh oh!

SteveL-MSFT commented Jul 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT Jul 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT Jul 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT commented Jul 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT commented Aug 1, 2017

Uh oh!

SteveL-MSFT commented Aug 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adityapatwardhan commented Aug 8, 2017

Uh oh!

adityapatwardhan commented Aug 8, 2017

Uh oh!

mirichmo commented Aug 9, 2017

Uh oh!

ltamaster commented Nov 22, 2017

Uh oh!

SteveL-MSFT commented Nov 22, 2017

Uh oh!

iSazonov commented Nov 23, 2017

Uh oh!

SteveL-MSFT commented Nov 23, 2017

Uh oh!

iSazonov commented Nov 24, 2017

Uh oh!

Reviewers

Assignees

Labels

SteveL-MSFT Jul 26, 2017 •

edited

Loading

SteveL-MSFT Jul 26, 2017 •

edited

Loading