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

Update script to get PSVersion from $PSVersionTable#5045

Merged
adityapatwardhan merged 5 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
daxian-dbw:fixremotescriptdaxian-dbw/PowerShell:fixremotescriptCopy head branch name to clipboard
Oct 9, 2017
Merged

Update script to get PSVersion from $PSVersionTable#5045
adityapatwardhan merged 5 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
daxian-dbw:fixremotescriptdaxian-dbw/PowerShell:fixremotescriptCopy head branch name to clipboard

Conversation

@daxian-dbw

@daxian-dbw daxian-dbw commented Oct 7, 2017

Copy link
Copy Markdown
Member

The file powershell.version has been removed as we now figure out PSVersion and GitCommitId from the ProductVersion of S,M.A.dll. Update the script to get the PSVersion directly from $PSVersionTable.PSVersion.

SteveL-MSFT
SteveL-MSFT requested changes Oct 7, 2017
View reviewed changes
Comment thread src/powershell-native/Install-PowerShellRemoting.ps1 Outdated
[ValidateNotNullOrEmpty()]
[parameter(Mandatory = $true, ParameterSetName = "ByPath")]
[string]
$PowerShellVersion = "6.0.0-alpha.8"

@SteveL-MSFT SteveL-MSFT Oct 7, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If the path is provided, we should get the powershell version using powershell -vand get rid of this parameter

@adityapatwardhan adityapatwardhan Oct 9, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agree.

@daxian-dbw daxian-dbw Oct 9, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sounds good. Will make the change.

@iSazonov

iSazonov commented Oct 7, 2017

Copy link
Copy Markdown
Collaborator

I wonder why the script is in src/powershell-native directory?

adityapatwardhan
adityapatwardhan approved these changes Oct 9, 2017
View reviewed changes

@adityapatwardhan adityapatwardhan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approved with minor comments.

Comment thread src/powershell-native/Install-PowerShellRemoting.ps1 Outdated
[ValidateNotNullOrEmpty()]
[parameter(Mandatory = $true, ParameterSetName = "ByPath")]
[string]
$PowerShellVersion = "6.0.0-alpha.8"

@adityapatwardhan adityapatwardhan Oct 9, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agree.

@daxian-dbw

daxian-dbw commented Oct 9, 2017

Copy link
Copy Markdown
Member Author

@iSazonov This script is to register WinRM endpoint configuration to make pwrshplugin.dll work for powershell core remoting. It's bundled together with pwrshplugin.dll in psrp.windows nuget package.

@mirichmo

mirichmo commented Oct 9, 2017

Copy link
Copy Markdown
Member

A few clean up items since we are touching this file:

  1. Line 6 should be removed. It is no longer relevant.
  2. The comment on line 39 should be cleaned up to be "powershell.6.0.0-beta.3"
  3. Line 78 should be uncommented. The registry file doesn't get cleaned up during MSI uninstallation
  4. The comment on line 116 should be moved to line 3 to become the introductory description since it provides an overview of the script. Right now, it is buried in the middle and hard to find.

mirichmo
mirichmo suggested changes Oct 9, 2017
View reviewed changes
Comment thread src/powershell-native/Install-PowerShellRemoting.ps1
reg.exe import .\$fileName

# Clean up
# Remove-Item .\$fileName

@mirichmo mirichmo Oct 9, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Clean up should still take place. Just remove the "#" on line 74

@daxian-dbw daxian-dbw Oct 9, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Added that line back.

@daxian-dbw

daxian-dbw commented Oct 9, 2017

Copy link
Copy Markdown
Member Author

@SteveL-MSFT Your comment has been addressed. Can you please take another look?
@adityapatwardhan The failure of macOS is not related to this PR. #5065 will fix macOS build.

SteveL-MSFT
SteveL-MSFT approved these changes Oct 9, 2017
View reviewed changes

@SteveL-MSFT SteveL-MSFT left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

adityapatwardhan
adityapatwardhan approved these changes Oct 9, 2017
View reviewed changes

@adityapatwardhan adityapatwardhan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@adityapatwardhan

adityapatwardhan commented Oct 9, 2017

Copy link
Copy Markdown
Member

Restarted MacOS build.

@daxian-dbw

daxian-dbw commented Oct 9, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

@adityapatwardhan The build is still failing, but it's not related to this PR.

@adityapatwardhan adityapatwardhan merged commit 55ce5dc into PowerShell:master Oct 9, 2017
@daxian-dbw daxian-dbw deleted the fixremotescript branch October 9, 2017 23:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SteveL-MSFT SteveL-MSFT SteveL-MSFT approved these changes

@adityapatwardhan adityapatwardhan adityapatwardhan approved these changes

+1 more reviewer

@mirichmo mirichmo mirichmo approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@adityapatwardhan adityapatwardhan

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@daxian-dbw @iSazonov @mirichmo @adityapatwardhan @SteveL-MSFT
Morty Proxy This is a proxified and sanitized view of the page, visit original site.