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

Installer detection improvements #111

Merged
merged 13 commits into from
Nov 22, 2021
Merged

Conversation

Trenly
Copy link
Owner

@Trenly Trenly commented Nov 18, 2021

@jedieaston @OfficialEsco

Making some big changes to how the installer entry is built out for new manifests. If you wouldn't mind doing some testing, I'm not sure how stable it is. I think it works, but I haven't gone through every possible use case

@OfficialEsco
Copy link
Collaborator

Oooo found a issue when creating a new package, the ProductCode field is not removed when no ProductCode is enterted

  ProductCode: ""

Also, we should probably clear the ReleaseNotes and ReleaseNotesUrl field

@Trenly
Copy link
Owner Author

Trenly commented Nov 18, 2021

Oooo found a issue when creating a new package, the ProductCode field is not removed when no ProductCode is enterted

  ProductCode: ""

Also, we should probably clear the ReleaseNotes and ReleaseNotesUrl field

I've got a separate branch going for release notes things; I'll merge that into this one and try and fix that product code issue

@OfficialEsco
Copy link
Collaborator

I've got a separate branch going for release notes things; I'll merge that into this one

We don't actually have any ReleaseNote or ReleaseNotesUrl metadata tho, so i don't think its too critical, better to merge it when we can input the metadata

Also the ReleaseNotes field sounds scary working with in Powershell

@Trenly
Copy link
Owner Author

Trenly commented Nov 18, 2021

I've got a separate branch going for release notes things; I'll merge that into this one

We don't actually have any ReleaseNote or ReleaseNotesUrl metadata tho, so i don't think its too critical, better to merge it when we can input the metadata

Also the ReleaseNotes field sounds scary working with in Powershell

It shouldn't be too bad honestly. Its just a string value thats up to 10k characters with multiline support

I'm more worried about ReleaseDate, since that is just a date. Do you know if there is a specific format needed for that? I'm thinking its just yyyy-MM-dd but I'm not 100% sure

@OfficialEsco
Copy link
Collaborator

I'm more worried about ReleaseDate, since that is just a date. Do you know if there is a specific format needed for that? I'm thinking its just yyyy-MM-dd but I'm not 100% sure

"ReleaseDate": {
      "type": [ "string", "null" ],
      "format": "date",
      "description": "The installer release date"
    }

Quick testing shows ANYTHING is valid, but we should use (tbh force) yyyy-MM-dd yes

@Trenly
Copy link
Owner Author

Trenly commented Nov 18, 2021

I'm more worried about ReleaseDate, since that is just a date. Do you know if there is a specific format needed for that? I'm thinking its just yyyy-MM-dd but I'm not 100% sure

"ReleaseDate": {
      "type": [ "string", "null" ],
      "format": "date",
      "description": "The installer release date"
    }

Quick testing shows ANYTHING is valid, but we should use (tbh force) yyyy-MM-dd yes

Perfect; I have it set up (again, different branch) to try and parse whatever the user enters into that format. So they could enter December 30 2021 and it should parse the value to 2021-12-30

@jedieaston
Copy link
Collaborator

jedieaston commented Nov 18, 2021

BTW, the format: "date" means that the date has to be ISO8601, i.e. yyyy-MM-dd. https://json-schema.org/understanding-json-schema/reference/string.html#dates-and-times.

@OfficialEsco
Copy link
Collaborator

BTW, the format: "date" means that the date has to be ISO8601, i.e. yyyy-MM-dd. https://json-schema.org/understanding-json-schema/reference/string.html#dates-and-times.

makes sense, seems like winget version 1.2.3131 can't validate it correctly then, cuz i could type anything and it would pass 🤷‍♂️

@OfficialEsco
Copy link
Collaborator

Got another bug when creating a new package with 2 installers, microsoft#35217

The script didn’t ask for the unknown archietecture so it just used x64

@Trenly
Copy link
Owner Author

Trenly commented Nov 18, 2021

Got another bug when creating a new package with 2 installers, microsoft#35217

The script didn’t ask for the unknown archietecture so it just used x64

I haven’t looked fully yet, but it seems like this would occur even when on microsoft/master because the packageidentifier has 64 in it?

@Trenly Trenly mentioned this pull request Nov 18, 2021
@vedantmgoyal9
Copy link

@Trenly Does 9e74f09 resolves #28?

@Trenly
Copy link
Owner Author

Trenly commented Nov 19, 2021

@Trenly Does 9e74f09 resolves #28?

Almost; I still have to figure out the auto update, but yes. Its probably good enough to call it resolved

Copy link

@vedantmgoyal9 vedantmgoyal9 left a comment

Choose a reason for hiding this comment

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

Typo and Color

Tools/YamlCreate.ps1 Outdated Show resolved Hide resolved
Copy link
Collaborator

@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.

Pretty sure this is approvable at this state 🤔 Unless you want to do more

@Trenly Trenly merged commit 03d0f11 into v2.1.0 Nov 22, 2021
@Trenly Trenly deleted the InstallerDetectionImprovements branch November 22, 2021 12:26
Trenly added a commit that referenced this pull request Nov 30, 2021
* Prepare file for 1.1.0 manifests - #31

* Installer detection improvements (#111)

* Begin update in place

* Condense installer switch logic

* Fix Null values

* Prevent null checking

* Cleanup

* Fix missed variable

* Bump version

* Update function calls

* Fix empty ProductCode

* Confirm Architecture when Automatically Detected

* Remove Debug Line

* Add duplicate url check for Quick update

* Remove line used in debugging

* Remove unneeded comment

* Resolves #121

Co-authored-by: Vedant <[email protected]>
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.

4 participants