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

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

Merged
adityapatwardhan merged 4 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
hemisphera:hemisphera-obroot-#13347hemisphera/PowerShell:hemisphera-obroot-#13347Copy head branch name to clipboard
Aug 25, 2020
Merged

#13347: ObRoot: Use field if property does not exist#13375
adityapatwardhan merged 4 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
hemisphera:hemisphera-obroot-#13347hemisphera/PowerShell:hemisphera-obroot-#13347Copy head branch name to clipboard

Conversation

@hemisphera

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

Copy link
Copy Markdown
Contributor

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
Comment thread src/System.Management.Automation/engine/remoting/common/RunspaceConnectionInfo.cs Outdated

@iSazonov iSazonov Aug 7, 2020

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

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

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. Thanks!

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

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

@PaulHigin PaulHigin Aug 10, 2020

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 think we should throw the RemotingErrorIdStrings.CannotGetHostInteropTypes exception here, same as above.

Comment thread src/System.Management.Automation/engine/remoting/common/RunspaceConnectionInfo.cs Outdated

@PaulHigin PaulHigin Aug 10, 2020

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

@PaulHigin PaulHigin left a comment

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
Thanks!

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

SteveL-MSFT commented Aug 13, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

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

@adityapatwardhan

adityapatwardhan commented Aug 13, 2020

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

ghost commented Aug 21, 2020

Copy link
Copy Markdown

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

adityapatwardhan commented Aug 25, 2020

Copy link
Copy Markdown
Member

@hemisphera Thank you for your contribution!

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

ghost commented Sep 8, 2020

Copy link
Copy Markdown

🎉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

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

@adityapatwardhan adityapatwardhan adityapatwardhan approved these changes

+2 more reviewers

@rjmholt rjmholt rjmholt approved these changes

@PaulHigin PaulHigin PaulHigin approved these changes

Reviewers whose approvals may not affect merge requirements

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
Morty Proxy This is a proxified and sanitized view of the page, visit original site.