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

SteveL-MSFT
Copy link
Member

@SteveL-MSFT SteveL-MSFT 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

Copy link
Contributor

@anmenaga anmenaga left a comment

Choose a reason for hiding this comment

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

LGTM

@SteveL-MSFT SteveL-MSFT force-pushed the formatps1xml-root-attributes branch 3 times, most recently from 345a15b to 7624a6e Compare October 11, 2018 00:52
@SteveL-MSFT
Copy link
Member Author

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

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

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

@anmenaga
Copy link
Contributor

anmenaga 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
Copy link
Contributor

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

Context.InitialSessionState.Formats.Clear();
foreach (var format in originalFormats)
{
Context.InitialSessionState.Formats.Add(format);
Copy link
Member

Choose a reason for hiding this comment

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

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

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

@adityapatwardhan
Copy link
Member

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

@SteveL-MSFT SteveL-MSFT force-pushed the formatps1xml-root-attributes branch 2 times, most recently from 785d273 to 0251824 Compare October 12, 2018 01:03
@SteveL-MSFT
Copy link
Member Author

@adityapatwardhan can you update your review?

@adityapatwardhan adityapatwardhan merged commit 8bca5a6 into PowerShell:master Oct 12, 2018
adityapatwardhan pushed a commit to adityapatwardhan/PowerShell that referenced this pull request Apr 9, 2019
@SteveL-MSFT SteveL-MSFT deleted the formatps1xml-root-attributes branch March 10, 2020 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants