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

Implement package pinning #2813

Merged
merged 62 commits into from
Feb 9, 2023
Merged

Implement package pinning #2813

merged 62 commits into from
Feb 9, 2023

Conversation

florelis
Copy link
Member

@florelis florelis commented Jan 3, 2023

This PR builds on top of #2769. I can close that PR in favor of this one, or we can merge that one first to make the changes smaller and easier to review :)

Changes in this PR (not in #2769):

  • Modified the CompositePackage to be able to hold more than one available package, each from a different source. The GetAvailableVersionKeys() function now can return versions from multiple sources, so the source field of PackageVersionKey is more relevant now. If the same version is available from multiple sources, they are returned in the order the available packages were added, which matches the previous behavior of just adding the first available package found.
  • Added a PinBehavior enum which is passed as an argument to an IPackage's functions for getting available versions. Only the CompositePackage uses it (as it needs both an installed and an available package) and it is used to determine whether we ignore pinned versions when listing the available versions or not. For example, for list we don't care about pins, for upgrade we do consider them, and for upgrade --include-pinned we care about them unless they have Pinning type.
    • Added this new argument all through the codebase. I'm open to making it have a default value, but I wanted to highlight all the places it is used to be sure I made the appropriate choice on each place.
  • Updated existing tests to new behaviors. For example, CompositeSource tests now need to ensure that the test packages have a reference to their source as it is used to determine if they are pinned.
  • There was already some work for pinning done for the RequiresExplicitUpgrade manifest field. This change is mostly independent of that one, as in my mind the pinning from RequiresExplicitUpgrade is a property of the installed package, whereas user-defined pins are properties of the available packages from each source, so they are handled at different places.

Closes #476
Related to #2611

Microsoft Reviewers: Open in CodeFlow

@florelis
Copy link
Member Author

florelis commented Jan 3, 2023

Here's a little screenshot of this thing in action:

image

src/AppInstallerCLICore/Commands/PinCommand.cpp Outdated Show resolved Hide resolved
src/AppInstallerCLICore/Workflows/PinFlow.cpp Outdated Show resolved Hide resolved
src/AppInstallerCommonCore/Errors.cpp Outdated Show resolved Hide resolved
@github-actions

This comment has been minimized.

@ghost ghost added the Issue-Feature This is a feature request for the Windows Package Manager client. label Jan 3, 2023
@florelis florelis marked this pull request as ready for review January 3, 2023 21:43
@florelis florelis requested a review from a team as a code owner January 3, 2023 21:43
@florelis
Copy link
Member Author

I moved the logic to filter out pinned versions out of CompositePackage's GetAvailableVersionKeys() and GetAvailableVersion(). Instead, now the version keys returned from GetAvailableVersionKeys() include the pinned state for that version. Versions from different sources may have different pin types, and versions that match a gating pin are not marked as pinned. Callers can the use that info to decide what to do.

I added another function like GetAvailableVersion() that also returns the pinned state.

I kept the PinBehavior argument for GetLatestAvailableVersion() because the logic there is more complicated and so it is harder to have each caller do it.

I'm still missing using the --force argument, and the messages that I'm exposing could probably be more specific.

@florelis
Copy link
Member Author

florelis commented Feb 6, 2023

/azp run

@azure-pipelines
Copy link

Pull request contains merge conflicts.

# Conflicts:
#	src/AppInstallerCLICore/Commands/UpgradeCommand.cpp
#	src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw
#	src/AppInstallerSharedLib/Errors.cpp
#	src/AppInstallerSharedLib/Public/AppInstallerErrors.h
src/AppInstallerCLIE2ETests/Pinning.cs Outdated Show resolved Hide resolved
src/AppInstallerCLIE2ETests/Pinning.cs Show resolved Hide resolved
src/AppInstallerCLIE2ETests/Pinning.cs Outdated Show resolved Hide resolved
src/AppInstallerRepositoryCore/CompositeSource.cpp Outdated Show resolved Hide resolved
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:

@florelis florelis merged commit b21389c into microsoft:master Feb 9, 2023
@florelis florelis deleted the pinningLogic branch February 9, 2023 00:11
@felipecrs
Copy link
Contributor

That's great news! I can't wait to test it :)

@denelon
Copy link
Contributor

denelon commented Feb 9, 2023

We're checking to see if any other PRs are super close to done, and we'll get a preview build out quickly™️.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Feature This is a feature request for the Windows Package Manager client.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pin a package
5 participants