Skip to content

Update script to get PSVersion from $PSVersionTable #5045

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
adityapatwardhan merged 5 commits into from
Oct 9, 2017
Merged

Update script to get PSVersion from $PSVersionTable #5045

adityapatwardhan merged 5 commits into from
Oct 9, 2017

Conversation

@daxian-dbw
Copy link
Member

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

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
src/powershell-native/Install-PowerShellRemoting.ps1 Outdated
[ValidateNotNullOrEmpty()]
[parameter(Mandatory = $true, ParameterSetName = "ByPath")]
[string]
$PowerShellVersion = "6.0.0-alpha.8"
Copy link
Member

@SteveL-MSFT SteveL-MSFT Oct 7, 2017

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

Copy link
Member

@adityapatwardhan adityapatwardhan Oct 9, 2017

Choose a reason for hiding this comment

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

I agree.

Copy link
Member Author

@daxian-dbw daxian-dbw Oct 9, 2017

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
Copy link
Collaborator

iSazonov commented Oct 7, 2017

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

adityapatwardhan
adityapatwardhan approved these changes Oct 9, 2017
View reviewed changes
Copy link
Member

@adityapatwardhan adityapatwardhan left a comment

Choose a reason for hiding this comment

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

Approved with minor comments.

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

@adityapatwardhan adityapatwardhan Oct 9, 2017

Choose a reason for hiding this comment

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

I agree.

@daxian-dbw
Copy link
Member Author

daxian-dbw commented Oct 9, 2017

@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
Copy link
Member

mirichmo commented Oct 9, 2017

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
src/powershell-native/Install-PowerShellRemoting.ps1
reg.exe import .\$fileName

# Clean up
# Remove-Item .\$fileName
Copy link
Member

@mirichmo mirichmo Oct 9, 2017

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

Copy link
Member Author

@daxian-dbw daxian-dbw Oct 9, 2017

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
Copy link
Member Author

daxian-dbw commented Oct 9, 2017

@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
Copy link
Member

@SteveL-MSFT SteveL-MSFT left a comment

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
Copy link
Member

@adityapatwardhan adityapatwardhan left a comment

Choose a reason for hiding this comment

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

LGTM

@adityapatwardhan
Copy link
Member

adityapatwardhan commented Oct 9, 2017

Restarted MacOS build.

@daxian-dbw
Copy link
Member Author

daxian-dbw commented Oct 9, 2017 •
edited
Loading

@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