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

Remove zip installer type from 1.0 and 1.1 manifests #3006

Merged
merged 2 commits into from
Feb 24, 2023

Conversation

ryfu-msft
Copy link
Contributor

@ryfu-msft ryfu-msft commented Feb 23, 2023

Continuation of #2996

Removes zip installer type from 1.0 and 1.1 manifests as zip is not supported until 1.4 client.
Also fixes the unit tests accordingly so that they do not rely on the zip installertype.

Microsoft Reviewers: Open in CodeFlow

@ryfu-msft ryfu-msft requested a review from a team as a code owner February 23, 2023 19:57
@Trenly
Copy link
Contributor

Trenly commented Feb 23, 2023

Holey smokes, I didn’t see they were implemented so many places! Thanks for catching my miss

@ryfu-msft
Copy link
Contributor Author

ryfu-msft commented Feb 23, 2023

Holey smokes, I didn’t see they were implemented so many places! Thanks for catching my miss

Nah, that's my mistake. I didn't realize the tests also were also affected.

@@ -4,14 +4,10 @@ Version: 1.7.32
Publisher: Microsoft
License: MIT License
Description: A complex system reference test file.
InstallerType: Zip
InstallerType: EXE
Copy link
Member

Choose a reason for hiding this comment

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

Nit: You use inconsistent case (EXE vs exe) on these

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed all the installer types in this test manifest to lowercase to match other yamls.

Comment on lines 11 to 14
- InstallerType: Zip
Arch: x86
Url: https://rubengustorage.blob.core.windows.net/publiccontainer/msixsdkx86.zip
Sha256: 69D84CA8899800A5575CE31798293CD4FEBAB1D734A07C2E51E56A28E0DF8C82
Copy link
Member

Choose a reason for hiding this comment

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

I'm assuming you made sure that we don't lose coverage by removing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the v1.4 manifest tests cover this scenario and ensure that fields from the root get passed to the installer if they are not defined.

REQUIRE(installer1.Scope == ScopeEnum::User);
REQUIRE(installer1.PackageFamilyName == "");
REQUIRE(installer1.ProductCode == "");
REQUIRE(installer1.ProductCode == "{Foo}");
Copy link
Member

Choose a reason for hiding this comment

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

Why did this one change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think for the older manifests, fields were not passed to the zip installer but that behavior has since changed. Since I changed the installertype to EXE, the productCode specified from the root is now being passed to the installer node.


// MSIX installer does inherit
REQUIRE(manifest.Installers[1].BaseInstallerType == InstallerTypeEnum::Msix);
Copy link
Member

Choose a reason for hiding this comment

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

It's probably not worth changing, but we could do something like

auto& installer = manifest.Installer[0];
REQUIRE(installer.BaseInstallerType == InstallerTypeEnum::Msix);
...

so that we don't have to change the index in all places. Though I doubt we'll have to do it again...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to suggested so that we never have to think about this again :)

@ryfu-msft ryfu-msft merged commit 7795d34 into microsoft:master Feb 24, 2023
@ryfu-msft ryfu-msft deleted the fixManifestTests branch February 24, 2023 00:58
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.

3 participants