Add empty `pin` command #2733

florelis · 2022-12-02T00:16:14Z

This change adds an empty pin command. Actual implementation of the command will come as a separate PR. This change includes:

Definition of the pin command and its sub-commands add, remove, list, reset. These definitions contain only the definition of arguments but are missing the actual execution code.
A new experimental feature flag to hide this command.
All the help resource strings for the command. Still pending all the resources that will be shown during actual execution.
Adds a source ls alias for the source list command.
Changes an existing string to make it consistent with the rest.

Microsoft Reviewers: Open in CodeFlow

florelis · 2022-12-02T00:16:48Z

Related to #476 and #2611 but does not resolve them yet

src/AppInstallerCLICore/Commands/PinCommand.cpp

florelis · 2022-12-02T00:19:20Z

src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw

@@ -1357,7 +1357,8 @@ Please specify one of them using the `--source` option to proceed.</value>
    <value>Portable install failed; Cleaning up...</value>


I really want to sort this file alphabetically but I know VS is just going to mess it up again...

src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw

src/AppInstallerCommonCore/Public/winget/ExperimentalFeature.h

src/AppInstallerCLICore/Commands/SourceCommand.h

doc/Settings.md

src/AppInstallerCLICore/Commands/PinCommand.cpp

Trenly · 2022-12-02T22:37:17Z

@jsoref

jsoref · 2022-12-03T23:25:31Z

@Trenly: interesting, when I tested it, I got a very different result: check-spelling-sandbox#2 (comment) (this comment has since been collapsed, but you can expand it).

From looking at the logs (here and there), I suspect that checkout-merge step is not doing the right thing tm.

Here'a a rebased run:

https://github.com/check-spelling/winget-cli/pull/2/files#file-src-powershell-microsoft-winget-client-examples-sample_findpackage-ps1-L4

And here's me trying to reproduce this PR as it was:
check-spelling-sandbox#3

I'm going to have to investigate the checkout-merge logic, it feels like it isn't quite behaving.

Can I ask that this be rebased after #2739 is merged? (Preferably squashing 74a97e6 into 1ede614 -- when I did a rebase, I tripped on a conflict there...)

florelis · 2022-12-05T22:15:31Z

Thanks for looking into this @jsoref !

Can I ask that this be rebased after #2739 is merged?

Rebased. Looks like it found one unrecognized word but I'm not planning on merging it with that line so I'll leave it like this for now.

yao-msft · 2022-12-07T03:04:10Z

doc/Settings.md

+
+### pinning
+
+This feature enables the ability to pin packages to a specific version to prevent the Windows Package Manager from updating them.


nit: it does not have to be a specific version

yao-msft · 2022-12-07T03:12:42Z

src/AppInstallerCLICore/Commands/SourceCommand.h

@@ -37,7 +37,7 @@ namespace AppInstaller::CLI

    struct SourceListCommand final : public Command
    {
-        SourceListCommand(std::string_view parent) : Command("list", { "ls" }, parent) {}
+        SourceListCommand(std::string_view parent) : Command("list", parent) {}


Will this actually remove the "ls" change done in previous pr during merging?

Yeah, I messed up the rebase. I'll revert this.

yao-msft · 2022-12-07T03:31:58Z

src/AppInstallerCLICore/Commands/PinCommand.cpp

+            Argument::ForType(Args::Type::Source),
+            Argument::ForType(Args::Type::CustomHeader),
+            Argument::ForType(Args::Type::AcceptSourceAgreements),
+            Argument::ForType(Args::Type::Force),


Curious what is the scenario for --force in pin add?

The scenario I was thinking of is overwriting the pin. Like winget pin add Pkg adds a pin, then winget pin add Pkg --version 1.* would fail because it exists, but if you add --force, it overwrites it. But honestly, I just added it because it was in the spec and I was hoping you had a scenario in mind :D

That makes sense. I guess I thought about it some time ago and now my memory does not serve me well.

Even more curious - should --force be added to the list of generic arguments (like --wait and --verbose-logs)? In the worst case, it doesn't change the behavior, and in most cases it will remove a few lines of code.

yao-msft · 2022-12-07T03:36:59Z

src/AppInstallerCLICore/Commands/PinCommand.cpp

+            Argument::ForType(Args::Type::Exact),
+            Argument::ForType(Args::Type::CustomHeader),
+            Argument::ForType(Args::Type::AcceptSourceAgreements),
+            Argument::ForType(Args::Type::Force),


Also curious what is the scenario for --force in pin remove here?

I don't know, I added it because it was in the spec. I can remove it until/unless we have a use for it. The only idea that comes to mind is having the command work for single pins and needing --force if multiple pins match.

florelis requested a review from a team as a code owner December 2, 2022 00:16

florelis commented Dec 2, 2022

View reviewed changes

This comment was marked as resolved.

Sign in to view

Trenly reviewed Dec 2, 2022

View reviewed changes

doc/Settings.md Outdated Show resolved Hide resolved

src/AppInstallerCLICore/Commands/PinCommand.cpp Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

florelis added 8 commits December 5, 2022 14:07

Add empty skeleton for command with gating experimental feature

7d6b265

Add command arguments

fa20950

Add resource strings

cfc29a8

Fix hex code

4b64439

Fix value-comment switch

f17d135

Update settings.md to include pinning

c60bd62

Undo changes moved to new PR

24ba038

PR comments

cbb64b9

florelis force-pushed the pinningSkeleton branch from cc514f7 to cbb64b9 Compare December 5, 2022 22:12

This comment has been minimized.

Sign in to view

Remove unneeded comment

9b9e4e3

yao-msft approved these changes Dec 7, 2022

View reviewed changes

Trenly mentioned this pull request Dec 7, 2022

Add Experimental Argument for UninstallPrevious in Upgrade Flow #2755

Merged

2 tasks

PR comments

2157ee6

florelis merged commit 291651f into microsoft:master Dec 8, 2022

florelis deleted the pinningSkeleton branch December 8, 2022 21:24

		@@ -1357,7 +1357,8 @@ Please specify one of them using the `--source` option to proceed.</value>
		<value>Portable install failed; Cleaning up...</value>


		### pinning

		This feature enables the ability to pin packages to a specific version to prevent the Windows Package Manager from updating them.

Add empty pin command #2733

Add empty pin command #2733

Uh oh!

Conversation

florelis commented Dec 2, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Microsoft Reviewers: Open in CodeFlow

Uh oh!

florelis commented Dec 2, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

Uh oh!

This comment was marked as resolved.

Trenly commented Dec 2, 2022

Uh oh!

jsoref commented Dec 3, 2022

Uh oh!

This comment has been minimized.

florelis commented Dec 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Add empty `pin` command #2733

Add empty `pin` command #2733

florelis commented Dec 2, 2022 •

edited by ghost

Loading

florelis commented Dec 5, 2022 •

edited

Loading