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

Use package tracking data to correlate in CompositeSource #1671

Merged
merged 4 commits into from
Nov 5, 2021

Conversation

JohnMcPMS
Copy link
Member

@JohnMcPMS JohnMcPMS commented Nov 3, 2021

Change

This change leverages the tracking data that is stored per source to correlate installed packages to available packages. This will currently provide no additional benefit with the winget source, as we aren't doing the work yet to determine the best fit system artifact when none of our current correlation attempts succeed. It will help the msstore source to correlate as that is currently lacking in support to search system references.

Validation

Existing regression tests and new tests added for the tracking catalog. Modified the tracking catalog to enable the existing test infrastructure to inject custom data.

Microsoft Reviewers: Open in CodeFlow

@JohnMcPMS JohnMcPMS requested a review from a team as a code owner November 3, 2021 22:32
@github-actions

This comment has been minimized.

static PackageTrackingCatalog CreateForSource(const Source& source);

// Removes the package tracking catalog for a given source.
// Removes the package tracking catalog for a given source identifier.
static void RemoveForSource(const std::string& identifier);
Copy link
Contributor

@yao-msft yao-msft Nov 4, 2021

Choose a reason for hiding this comment

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

RemoveForSource

I saw this is only used in unit tests now, will there be usage in the product code in the future? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure, but since I wanted to use it in the tests I implemented it here. We could call it as part of removing a source, but I wasn't sure if that was the right thing to do.

{
auto compositePackage = std::make_shared<CompositePackage>(
std::move(installedPackage),
GetTrackedPackageFromAvailableSource(result, source, match.Package->GetProperty(PackageProperty::Id)));
Copy link
Contributor

Choose a reason for hiding this comment

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

GetTrackedPackageFromAvailableSource

This might return null, in that case, we prevent the later available package to be associated with this installed package, I guess it'll be fixed when we have support for 1 -> n installed to available mapping in the future

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, this might cause some upgrade cases to report "no applicable upgrades found" incorrectly. Looks like we may want to fix the 1 -> n mapping sooner.

Copy link
Member Author

Choose a reason for hiding this comment

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

We need to think about the behavior in that case. We haven't really laid down how the association will work. In my opinion, we should not attempt to upgrade in the case that the original source no longer contains the package, but another one does. We should instead inform the user of that fact so that they can decide what to do. If they did install/upgrade into the other source, then the association would be updated and we would start upgrading from the new location in the future.

Copy link
Contributor

@yao-msft yao-msft left a comment

Choose a reason for hiding this comment

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

:shipit:

@JohnMcPMS JohnMcPMS merged commit 2de86a4 into microsoft:master Nov 5, 2021
@JohnMcPMS JohnMcPMS deleted the correlatetracked branch November 5, 2021 18:42
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