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

Add alternate url support for some predefined sources #2970

Merged
merged 1 commit into from
Feb 16, 2023

Conversation

JohnMcPMS
Copy link
Member

@JohnMcPMS JohnMcPMS commented Feb 15, 2023

Sometimes bad things happen; even when you thought you had everything set up properly it might turn out that you didn't. The best laid plans, etc. If there is a way to continue functioning, even if it is less efficient, shouldn't we use that?

Change

Adds an alternate base URL for the sources behind cdn.winget.microsoft.com. In the event that the files hosted there are inaccessible or corrupted, the alternate URL will be used instead. This mechanism does rely on the fact that these are basically two names for the exact same set of data, which is true as the alternate is the upstream store for the CDN itself.

Additionally, we now require preindexed packaged sources to contain the manifest hash, rather than just using it if present.

Finally, this change adds a setting that can be used to disable the alternate URL for the winget source. I intentionally did not add it to the schema or documentation, as I don't think it is something that should actually be set by anyone unless there is an explicit reason to do so (such as for testing purposes).

Validation

Manually validated by changing the primary URL to a non-existent one. The error was present in the logs, but the user experience was not impacted. Tested the setting in this case to ensure that it would cause a failure.

Microsoft Reviewers: Open in CodeFlow
Microsoft Reviewers: Open in CodeFlow

@JohnMcPMS JohnMcPMS requested a review from a team as a code owner February 15, 2023 04:36
@@ -325,6 +331,7 @@ namespace AppInstaller::Repository
details.Name = s_Source_DesktopFrameworks_Name;
details.Type = Microsoft::PreIndexedPackageSourceFactory::Type();
details.Arg = s_Source_DesktopFrameworks_Arg;
details.AlternateArg = s_Source_DesktopFrameworks_AlternateArg;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it intentional to not check settings for this one?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I was intentionally not affecting the platform one because it is used in an automated fashion. If we had a GP, then I would have it affected then.

}

const std::string& PackageLocation() const { return m_packageLocation; }
Msix::MsixInfo& MsixInfo() { return *m_msixInfo; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be returning const MsixInfo& too?

Copy link
Member Author

Choose a reason for hiding this comment

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

The functions that it is passed to take MsixInfo&, so I am not returning it as const. Likely MsixInfo just isn't const correct.
Probably my fault. But not trying to correct that with this change.

@JohnMcPMS JohnMcPMS merged commit 428ffb5 into microsoft:master Feb 16, 2023
@JohnMcPMS JohnMcPMS deleted the source-fallback branch February 16, 2023 19:48
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.

2 participants