Skip to content

#13347: ObRoot: Use field if property does not exist #13375

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
merged 4 commits into from
Aug 25, 2020
Merged

#13347: ObRoot: Use field if property does not exist #13375

merged 4 commits into from
Aug 25, 2020

Conversation

hemisphera
Copy link
Contributor

@hemisphera hemisphera commented Aug 7, 2020 •
edited by iSazonov
Loading

PR Summary

Use field ObRoot if property ObRoot does not exist in GetContainerPropertiesInternal.

PR Context

Fix #13347

PR Checklist

@ghost ghost assigned adityapatwardhan Aug 7, 2020
iSazonov
iSazonov reviewed Aug 7, 2020
View reviewed changes
src/System.Management.Automation/engine/remoting/common/RunspaceConnectionInfo.cs Outdated
@@ -3443,7 +3443,15 @@ private void GetContainerPropertiesInternal()
//
if (RuntimeId == Guid.Empty)
{
ContainerObRoot = (string)computeSystemPropertiesType.GetProperty("ObRoot").GetValue(computeSystemPropertiesHandle);
var pi = computeSystemPropertiesType.GetProperty("ObRoot");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please look a comment above for RuntimeId and follow the same pattern - check the field first.

@SteveL-MSFT SteveL-MSFT requested a review from PaulHigin August 10, 2020 21:08
SteveL-MSFT
SteveL-MSFT approved these changes Aug 10, 2020
View reviewed changes
Copy link
Member

@SteveL-MSFT SteveL-MSFT left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM. Thanks!

@hemisphera, you validated this works manually with a private build?

PaulHigin
PaulHigin suggested changes Aug 10, 2020
View reviewed changes
src/System.Management.Automation/engine/remoting/common/RunspaceConnectionInfo.cs Outdated

if (ContainerObRoot == null)
{
throw new ArgumentNullException(nameof(ContainerObRoot));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should throw the RemotingErrorIdStrings.CannotGetHostInteropTypes exception here, same as above.

src/System.Management.Automation/engine/remoting/common/RunspaceConnectionInfo.cs Outdated
@@ -3443,7 +3443,24 @@ private void GetContainerPropertiesInternal()
//
if (RuntimeId == Guid.Empty)
{
ContainerObRoot = (string)computeSystemPropertiesType.GetProperty("ObRoot").GetValue(computeSystemPropertiesHandle);
var obRootfieldInfo = computeSystemPropertiesType.GetField("ObRoot");
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add similar comment about Hyper-V team changing property to field, to make it clear to others maintaining this code.

@ghost ghost added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept labels Aug 10, 2020
PaulHigin
PaulHigin approved these changes Aug 11, 2020
View reviewed changes
Copy link
Contributor

@PaulHigin PaulHigin left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks!

@SteveL-MSFT SteveL-MSFT force-pushed the hemisphera-obroot-#13347 branch from f8d1fb2 to 4c74529 Compare August 13, 2020 20:41
@SteveL-MSFT
Copy link
Member

SteveL-MSFT commented Aug 13, 2020 •
edited
Loading

The codefactor issues are non-issues, so I think this PR is ready to go. @hemisphera, thanks for the contribution!

@adityapatwardhan
Copy link
Member

@hemisphera Please have a look at suggestion from @rjmholt

@ghost ghost added the Review - Needed The PR is being reviewed label Aug 21, 2020
@ghost
Copy link

ghost commented Aug 21, 2020

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Aug 21, 2020
@adityapatwardhan adityapatwardhan merged commit 126d942 into PowerShell:master Aug 25, 2020
@ghost ghost removed the Review - Needed The PR is being reviewed label Aug 25, 2020
@adityapatwardhan
Copy link
Member

@hemisphera Thank you for your contribution!

@iSazonov iSazonov added this to the 7.1.0-preview.7 milestone Aug 26, 2020
@ghost
Copy link

ghost commented Sep 8, 2020

🎉v7.1.0-preview.7 has been released which incorporates this pull request.:tada:

Handy links:

@ghost ghost mentioned this pull request Sep 8, 2020
Closed
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

@rjmholt rjmholt rjmholt approved these changes

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

@adityapatwardhan adityapatwardhan adityapatwardhan approved these changes

@PaulHigin PaulHigin PaulHigin approved these changes

Assignees

@adityapatwardhan adityapatwardhan

Labels
CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log
Projects
None yet
Milestone
7.1.0-preview.7
Development

Successfully merging this pull request may close these issues.

PSSessions with Containers throw NullReferenceException
7 participants
@hemisphera @SteveL-MSFT @adityapatwardhan @rjmholt @PaulHigin @iSazonov @Thomas-Torggler-EOS