Join GitHub today
GitHub is home to over 36 million developers working together to host and review code, manage projects, and build software together.
Sign upAllow root node of format.ps1xml to have attributes that are ignored #7987
Conversation
SteveL-MSFT
assigned
adityapatwardhan
Oct 10, 2018
SteveL-MSFT
requested review from
anmenaga and
adityapatwardhan
Oct 10, 2018
SteveL-MSFT
force-pushed the
SteveL-MSFT:formatps1xml-root-attributes
branch
3 times, most recently
Oct 11, 2018
This comment has been minimized.
This comment has been minimized.
|
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. |
This comment has been minimized.
This comment has been minimized.
|
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
requested review from
JamesWTruher and
PaulHigin
as
code owners
Oct 11, 2018
SteveL-MSFT
requested a review
from
daxian-dbw
Oct 11, 2018
This comment has been minimized.
This comment has been minimized.
|
@anmenaga can you re-review? Had to make additional fixes due to CI test failures |
iSazonov
referenced this pull request
Oct 11, 2018
Merged
Refactor module version/GUID comparison logic #7125
This comment has been minimized.
This comment has been minimized.
|
Looks good to me. |
This comment has been minimized.
This comment has been minimized.
|
@adityapatwardhan this contains fix for CI failures. Please give this PR a priority. |
adityapatwardhan
requested changes
Oct 11, 2018
| Context.InitialSessionState.Formats.Clear(); | ||
| foreach (var format in originalFormats) | ||
| { | ||
| Context.InitialSessionState.Formats.Add(format); |
This comment has been minimized.
This comment has been minimized.
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);
This comment has been minimized.
This comment has been minimized.
|
I'll submit the test-connection test fix as separate PR since it's affecting other PRs |
This comment has been minimized.
This comment has been minimized.
|
@SteveL-MSFT now that the test CI fix is merge, please remove the fix in this PR. |
SteveL-MSFT
added some commits
Oct 10, 2018
SteveL-MSFT
force-pushed the
SteveL-MSFT:formatps1xml-root-attributes
branch
2 times, most recently
to
0251824
Oct 12, 2018
This comment has been minimized.
This comment has been minimized.
|
@adityapatwardhan can you update your review? |
SteveL-MSFT commentedOct 10, 2018
•
edited
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
Pendingfrom a test that passes.Address one aspect of #7749
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.[feature]if the change is significant or affects feature tests