★ wanayoo — archive 1999 https://github.com/PowerShell/PowerShell/pull/7987Nouvelle recherche | Portail wanayoo
Skip to content
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.

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

Allow root node of format.ps1xml to have attributes that are ignored #7987

Merged

Conversation

Projects
None yet
3 participants
@SteveL-MSFT
Copy link
Member

commented Oct 10, 2018

PR Summary

Currently, if a format.ps1xml file has attributes on the <Configuration> node, it is a terminating error. We should allow for loose validation so that common attributes from other tooling is simply ignored if not understood. @PowerShell/powershell-committee had discussed this and agreed with making this change. Fix is to use a different method to find the root node that allows for attributes.

Types.ps1xml doesn't have this strict validation so no change was needed, but added tests for both format and types ps1xml.

There was a separate issue causing tests to fail. A separate test used an invalid ps1xml file with update-formatdata. Expectation is that an invalid file wouldn't be bound to the runspace so it didn't need to run in a separate runspace. However, the runspace initialsessionstate kept this file in its list so subsequent attempts to update-formatdata (like importing a module that contains ps1xml) will again attempt to update with the invalid ps1xml file and result in a confusing error. Fix is to revert initialsession state to previous list of format files if there is a failure updating the formats. Renamed some of the test ps1xml files to be unique to determine which was causing the failure.

Also removed Pending from a test that passes.

Address one aspect of #7749

PR Checklist

@anmenaga
Copy link
Contributor

left a comment

LGTM

@SteveL-MSFT SteveL-MSFT force-pushed the SteveL-MSFT:formatps1xml-root-attributes branch 3 times, most recently Oct 11, 2018

@SteveL-MSFT

This comment has been minimized.

Copy link
Member Author

commented Oct 11, 2018

Figured out cause of test failure, but not sure why yet. A different test is updating formatdata with invalid ps1xml. Somehow, this causes importing of some (but not all) modules to fail complaining about the invalid ps1xml file. Can repro it manually with 6.1.0 without my change so my change is unrelated, but looks like a real bug. Investigating further.

@SteveL-MSFT

This comment has been minimized.

Copy link
Member Author

commented Oct 11, 2018

It appears the problem is that the invalid format ps1xml still gets into the formattable of the session so when importing a module that has a format.ps1xml it fails to update due to the previously bad format.ps1xml file still being cached.

@SteveL-MSFT SteveL-MSFT requested a review from daxian-dbw Oct 11, 2018

@SteveL-MSFT

This comment has been minimized.

Copy link
Member Author

commented Oct 11, 2018

@anmenaga can you re-review? Had to make additional fixes due to CI test failures

@iSazonov iSazonov referenced this pull request Oct 11, 2018

Merged

Refactor module version/GUID comparison logic #7125

8 of 11 tasks complete
@anmenaga

This comment has been minimized.

Copy link
Contributor

commented Oct 11, 2018

Looks good to me.
On a side note, looks like DNS resolution fix is unrelated to this PR topic and probably should have gone into a separate PR to expedite merging it.

@anmenaga

This comment has been minimized.

Copy link
Contributor

commented Oct 11, 2018

@adityapatwardhan this contains fix for CI failures. Please give this PR a priority.

src/Microsoft.PowerShell.Commands.Utility/commands/utility/Update-TypeData.cs Outdated
Context.InitialSessionState.Formats.Clear();
foreach (var format in originalFormats)
{
Context.InitialSessionState.Formats.Add(format);

This comment has been minimized.

Copy link
@adityapatwardhan

adityapatwardhan Oct 11, 2018

Member

Context.InitialSessionState.Formats.Add() has an overload which takes an IEnumerable<T>
So this should be

Context.InitialSessionState.Formats.Clear();
Context.InitialSessionState.Formats.Add(originalFormats);
@SteveL-MSFT

This comment has been minimized.

Copy link
Member Author

commented Oct 11, 2018

I'll submit the test-connection test fix as separate PR since it's affecting other PRs

@adityapatwardhan

This comment has been minimized.

Copy link
Member

commented Oct 11, 2018

@SteveL-MSFT now that the test CI fix is merge, please remove the fix in this PR.

@SteveL-MSFT SteveL-MSFT force-pushed the SteveL-MSFT:formatps1xml-root-attributes branch 2 times, most recently to 0251824 Oct 12, 2018

@SteveL-MSFT

This comment has been minimized.

Copy link
Member Author

commented Oct 12, 2018

@adityapatwardhan can you update your review?

@adityapatwardhan adityapatwardhan merged commit 8bca5a6 into PowerShell:master Oct 12, 2018

8 checks passed

CodeFactor No issues found.
Details
PowerShell-CI-linux #PR-7987-20181012.02 succeeded
Details
PowerShell-CI-macos #PR-7987-20181012.02 succeeded
Details
PowerShell-CI-spelling #PR-7987-20181012.02 succeeded
Details
PowerShell-CI-windows #PR-7987-20181012.02 succeeded
Details
WIP ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla All CLA requirements met.
Details

adityapatwardhan added a commit to adityapatwardhan/PowerShell that referenced this pull request Apr 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.