GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Allow root node of format.ps1xml to have attributes that are ignored by SteveL-MSFT · Pull Request #7987 · PowerShell/PowerShell

Conversation

@SteveL-MSFT

@SteveL-MSFT Steve Lee (SteveL-MSFT) commented Oct 10, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • Change is not breaking
  • Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
  • This PR is ready to merge and is not Work in Progress.
    • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.
  • User-facing changes
    • Not Applicable
    • OR
    • User-facing Documentation needed
      • Issue filed - Issue link:
  • Testing - New and feature
    • Not Applicable or can only be tested interactively
    • OR
    • Make sure you've added a new test if existing tests do not effectively test the code changed
      • Add [feature] if the change is significant or affects feature tests

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
PixleProgrambetaxl reacted with hooray emoji
All reactions
  • 🎉 1 reaction
@SteveL-MSFT Steve Lee (SteveL-MSFT) assigned Aditya Patwardhan (adityapatwardhan) Oct 10, 2018
@SteveL-MSFT
Steve Lee (SteveL-MSFT) requested review from Aditya Patwardhan (adityapatwardhan) and Andrew (anmenaga) October 10, 2018 20:54
anmenaga
Andrew (anmenaga) approved these changes Oct 10, 2018
View reviewed changes

@anmenaga Andrew (anmenaga) left a comment

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@SteveL-MSFT
Steve Lee (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

Steve Lee (SteveL-MSFT) commented Oct 11, 2018

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@SteveL-MSFT

Steve Lee (SteveL-MSFT) commented Oct 11, 2018

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@SteveL-MSFT
Steve Lee (SteveL-MSFT) requested review from James Truher (JamesWTruher) and Paul Higinbotham (PaulHigin) as code owners October 11, 2018 03:47
@SteveL-MSFT
Steve Lee (SteveL-MSFT) requested a review from Dongbo Wang (daxian-dbw) October 11, 2018 03:50
@SteveL-MSFT

Steve Lee (SteveL-MSFT) commented Oct 11, 2018

Copy link
Copy Markdown
Member Author

Andrew (@anmenaga) can you re-review? Had to make additional fixes due to CI test failures
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov Ilya (iSazonov) mentioned this pull request Oct 11, 2018
Refactor module version/GUID comparison logic #7125
Merged
8 of 11 tasks
@anmenaga

Andrew (anmenaga) commented Oct 11, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@anmenaga

Andrew (anmenaga) commented Oct 11, 2018

Copy link
Copy Markdown

Aditya Patwardhan (@adityapatwardhan) this contains fix for CI failures. Please give this PR a priority.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
adityapatwardhan
Aditya Patwardhan (adityapatwardhan) requested changes Oct 11, 2018
View reviewed changes
Comment thread src/Microsoft.PowerShell.Commands.Utility/commands/utility/Update-TypeData.cs Outdated

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Oct 11, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
SteveL-MSFT reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@SteveL-MSFT

Steve Lee (SteveL-MSFT) commented Oct 11, 2018

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@adityapatwardhan

Aditya Patwardhan (adityapatwardhan) commented Oct 11, 2018

Copy link
Copy Markdown
Member

Steve Lee (@SteveL-MSFT) now that the test CI fix is merge, please remove the fix in this PR.
SteveL-MSFT reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Steve Lee (SteveL-MSFT) added 4 commits October 11, 2018 18:00
@SteveL-MSFT
allow root node of format.ps1xml to have attributes that are ignored
12be68c 
added tests for format and types
@SteveL-MSFT
give test.format.ps1xml files unique names to identify cause of failures
405724f
@SteveL-MSFT
if there is a failure processing formats, revert to original formats …
c76cf4b 
…for initialsessionstate
@SteveL-MSFT
address Aditya's feedback
0251824
@SteveL-MSFT
Steve Lee (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

Steve Lee (SteveL-MSFT) commented Oct 12, 2018

Copy link
Copy Markdown
Member Author

Aditya Patwardhan (@adityapatwardhan) can you update your review?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
adityapatwardhan
Aditya Patwardhan (adityapatwardhan) approved these changes Oct 12, 2018
View reviewed changes
@adityapatwardhan
Aditya Patwardhan (adityapatwardhan) merged commit 8bca5a6 into PowerShell:master Oct 12, 2018
Aditya Patwardhan (adityapatwardhan) pushed a commit to adityapatwardhan/PowerShell that referenced this pull request Apr 9, 2019
@SteveL-MSFT @adityapatwardhan
Allow root node of format.ps1xml to have attributes that are ignored (P…
561a720 
…owerShell#7987)
@SteveL-MSFT
Steve Lee (SteveL-MSFT) deleted the formatps1xml-root-attributes branch March 10, 2020 21:01
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@SteveL-MSFT @adityapatwardhan
Allow root node of format.ps1xml to have attributes that are ignored (P…
048eb14 
…owerShell#7987)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) adityapatwardhan approved these changes
@JamesWTruher James Truher (JamesWTruher) Awaiting requested review from JamesWTruher
@PaulHigin Paul Higinbotham (PaulHigin) Awaiting requested review from PaulHigin
@daxian-dbw Dongbo Wang (daxian-dbw) Awaiting requested review from daxian-dbw
+1 more reviewer
@anmenaga Andrew (anmenaga) anmenaga approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@adityapatwardhan Aditya Patwardhan (adityapatwardhan)

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@SteveL-MSFT @anmenaga @adityapatwardhan
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.