The Wayback Machine - https://web.archive.org/web/20250501033957/https://github.com/microsoft/winget-pkgs/pull/21204
Skip to content

Improve YAML Parsing #21204

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

Merged
merged 104 commits into from
Nov 3, 2021
Merged

Improve YAML Parsing #21204

merged 104 commits into from
Nov 3, 2021

Conversation

Trenly
Copy link
Contributor

@Trenly Trenly commented Jul 16, 2021

Summary

This PR updates the YamlCreate.ps1 script to be more fully-featured and uses powershell-yaml (See it on GitHub) to handle the parsing of keys and values

Major Updates

  • Script documentation added under /doc/tools/YamlCreate.md
  • PackageId, PackageVersion, Mode, and Settings parameters added as optional
  • Validation of inputs against the manifest schema
  • Ability to fill in PR parameters when submitting PR's automatically
    • Ability to link issues when submitting a PR
  • Quick update mode added for when only InstallerURL's change
  • Allows for locales other than en-US as the defaultLocale
  • User can customize their settings for the script
  • Checks for PR's that may be for the same change

Minor Updates

  • Bumped script version and updated header
  • Installer keys are sorted based on the order in the schema
  • Automatic detection of architecture and installer type where possible
  • Automatic detection of package family name, where possible
  • Changed the termination types to be exit when a user choice exits the script and throw when the script terminates unexpectedly
  • Throws descriptive exceptions with inner exceptions for debugging
  • Delete mode for removing package versions, with reason
  • Handles file names in a safer manner
  • Handles branch names in a safer manner
  • Uses quasi-unique branch names
  • Suppresses much of the git output which is not necessary to show


Resolves #21940
Resolves #21219

Microsoft Reviewers: Open in CodeFlow
@Trenly Trenly changed the title Yaml parsing Improve YAML Parsing Jul 16, 2021
@denelon denelon linked an issue Jul 16, 2021 that may be closed by this pull request
@wingetbot wingetbot added the PullRequest-Error PR is Invalid label Jul 16, 2021
@ghost
Copy link

ghost commented Jul 16, 2021

Hello @Trenly,
The package manager bot determined there was an issue with the pull request. Make sure the manifest files are under the manifests\partition\publisher\appname\version directory. The partition of the path must be the first letter of the publisher in lower-case.

Example:
Path: manifests / m / Microsoft / WindowsTerminal / 1.6.10571.0 / WindowsTerminal.yaml

For details on the error, see the details link below in the build pipeline.

You may also try the Windows Package Manager Manifest Creator

@ghost ghost added Needs: author feedback Needs-Author-Feedback This needs a response from the author. labels Jul 16, 2021
@ghost ghost removed Needs: author feedback Needs-Author-Feedback This needs a response from the author. PullRequest-Error PR is Invalid labels Jul 16, 2021
@Trenly
Copy link
Contributor Author

Trenly commented Jul 16, 2021

Something weird happened with the commit history when I tried to rebase on master. I revered back to before the rebase. I'll rebase to resolve the merge conflicts before going too much further

@wingetbot wingetbot added the Validation-Merge-Conflict Package could not be tested due to merge conflict. label Jul 17, 2021
@ghost
Copy link

ghost commented Jul 17, 2021

Hello @Trenly,
The PR could not be validated because there is a merge conflict which adds bad characters to the manifest. Please address the merge conflict and resubmit.

@ghost ghost added the Needs-Author-Feedback This needs a response from the author. label Jul 17, 2021
@ghost ghost removed Needs: author feedback Needs-Author-Feedback This needs a response from the author. labels Jul 19, 2021
@Trenly
Copy link
Contributor Author

Trenly commented Jul 19, 2021

@OfficialEsco I believe this is all I have left -

<#
TO-DO:
    - Handle writing null parameters as comments
    - Ensure licensing for powershell-yaml is met
#>

I'm not sure how to write comments with powershell-yaml, so that may be difficult. As far as I can tell, there aren't any licensing requirement for powershell-yaml since we aren't using the source code in any way, but I would like someone else to verify that.

Other than those two things, I believe this is ready for testing

@denelon
Copy link
Collaborator

denelon commented Jul 20, 2021

@Trenly is it this one?

@Trenly
Copy link
Contributor Author

Trenly commented Jul 20, 2021

@Trenly is it this one?

Yes, It is that one

Copy link
Contributor

@OfficialEsco OfficialEsco left a comment

Choose a reason for hiding this comment

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

Issue 1:

[Optional] Enter any Protocols the application provides a handler for. For example: http, https (Max 16)
Protocols:

[Optional] Enter any Commands or aliases to run the application. For example: msedge (Max 16)
Commands:

[Optional] List of additional non-zero installer success exit codes other than known default values by winget (Max 16)
Commands:

Feature Request:
Keep Metadata outside of hardcoded metadata (do we have to hardcode all the different metadata fields?)

Untested:
Does it work as expected with additional locales? (Copying them to the new version)

I'll keep testing it if i can find any non submitted versions which have gotten harder 😠

Edit: On last issue revealed itself at the end

Submit PR?
Do you want to submit your PR now?
[Y] Yes  [N] No (default is 'Y'): yUpdate
PS C:\Users\Esco\Github\winget-pkgs>
@ghost ghost added Needs: author feedback Needs-Author-Feedback This needs a response from the author. and removed Needs: author feedback Needs-Author-Feedback This needs a response from the author. labels Jul 20, 2021
@Trenly
Copy link
Contributor Author

Trenly commented Oct 16, 2021

@jedieaston @vedantmgoyal2009 I believe these are the only issues remaining:

RuleName                            Severity     ScriptName Line  Message                                                     
--------                            --------     ---------- ----  -------                                                     
PSReviewUnusedParameter             Warning      YamlCreate 886   The parameter 'InputObject' has been declared but not used. 
                                                 .ps1                                                                         
PSReviewUnusedParameter             Warning      YamlCreate 889   The parameter 'NoComments' has been declared but not used.  
                                                 .ps1                                                                         

I looked and I see these parameters being used, so I think they are false positives

@Trenly
Copy link
Contributor Author

Trenly commented Oct 17, 2021

@denelon I've left the commit history mostly in-tact up until this point; Would you have any objection if I rewrite the history of this branch to reduce the amount of clutter?

@jedieaston
Copy link
Contributor

If you rebase we might lose some of the commit data for the PRs in your fork (I think). If this will be squashed on merge anyway, do you need to clear it now?

@Trenly
Copy link
Contributor Author

Trenly commented Oct 17, 2021

If you rebase we might lose some of the commit data for the PRs in your fork (I think). If this will be squashed on merge anyway, do you need to clear it now?

It depends on how the rebase is performed; I just don’t know if it will be squash merged here or just merged

@denelon
Copy link
Collaborator

denelon commented Oct 18, 2021

We squash merge.

@Trenly
Copy link
Contributor Author

Trenly commented Oct 25, 2021

@denelon I know it's a big ask, but how long do you think review will take?

@denelon
Copy link
Collaborator

denelon commented Oct 25, 2021

I'll see if I can get someone to look at this today or tomorrow. We have a lot in flight right now. Sorry this is taking so long.

@Trenly
Copy link
Contributor Author

Trenly commented Oct 25, 2021

I'll see if I can get someone to look at this today or tomorrow. We have a lot in flight right now. Sorry this is taking so long.

No worries. I completely understand having a lot in flight. There is no rush on this, I understand it's a community submission and that you guys have a lot of other things to be working on. I appreciate all your help in getting this reviewed though, it truly means a lot

@Trenly
Copy link
Contributor Author

Trenly commented Nov 2, 2021

@jedieaston @OfficialEsco Would you mind reviewing this PR for @denelon ?

jedieaston
jedieaston previously approved these changes Nov 2, 2021
Copy link
Contributor

@jedieaston jedieaston left a comment

Choose a reason for hiding this comment

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

I ran through it one more time, there doesn't seem to be any outstanding uh-ohs that we haven't noted.

:shipit:

@ghost ghost added the Moderator-Approved One of the Moderators has reviewed and approved this PR label Nov 2, 2021
@ghost ghost removed the Moderator-Approved One of the Moderators has reviewed and approved this PR label Nov 2, 2021
@Trenly
Copy link
Contributor Author

Trenly commented Nov 3, 2021

There doesn't seem to be any outstanding uh-ohs that we haven't noted.

Found one from the latest fixes to the CLI; Patched it

@ghost ghost added the Moderator-Approved One of the Moderators has reviewed and approved this PR label Nov 3, 2021
@denelon denelon merged commit 72ac482 into microsoft:master Nov 3, 2021
@Trenly Trenly deleted the YamlParsing branch November 3, 2021 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Moderator-Approved One of the Moderators has reviewed and approved this PR Project-File Validation-Merge-Conflict Package could not be tested due to merge conflict.
8 participants