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

Print the upgrade table during upgrade --all #1866

Merged
merged 2 commits into from
Jan 22, 2022

Conversation

jedieaston
Copy link
Contributor

@jedieaston jedieaston commented Jan 20, 2022


Resolves #1858.

This PR changes the upgrade --all flow to print the table of available upgrades before running the upgrades, giving the user a more friendly list of upgrades (as opposed to several lines of Workflow::ReportManifestIdentityWithVersion).

I had to make a change to EnsurePackageAgreementsAcceptanceForMultipleInstallers, namely, that it now gets the manifest and checks to see if there are any agreements before printing anything. This is less efficient if there is an agreement, but since most packages don't have agreements right now, it shouldn't (on average) change performance. (Edit: this doesn't change performance really at all, see below).

I reused the upgrade table from winget upgrade for this too, if a different appearance would be better then I can make a new table. Speaking of appearance, I know that nobody from MS commented on that ticket, so if anyone thinks it is better the other way then I can close this.

Tested: manually.

image

Microsoft Reviewers: Open in CodeFlow

@jedieaston jedieaston requested a review from a team as a code owner January 20, 2022 16:18
@ghost ghost added the Issue-Feature This is a feature request for the Windows Package Manager client. label Jan 20, 2022
@yao-msft
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@yao-msft
Copy link
Contributor

This is less efficient if there is an agreement, but since most packages don't have agreements right now, it shouldn't (on average) change performance.
Actually, currently manifests with agreements will be blocked in winget-pkgs submission. But almost all packages from msstore source have agreements.

@yao-msft
Copy link
Contributor

        hasPackageAgreements |= !showContext.Get<Execution::Data::Manifest>().CurrentLocalization.Get<AppInstaller::Manifest::Localization::Agreements>().empty();

nit: this could now be hasPackageAgreements |= true;


Refers to: src/AppInstallerCLICore/Workflows/InstallFlow.cpp:211 in 9111a03. [](commit_id = 9111a03, deletion_comment = False)

@JohnMcPMS
Copy link
Member

This is less efficient if there is an agreement, but since most packages don't have agreements right now, it shouldn't (on average) change performance.

Actually, currently manifests with agreements will be blocked in winget-pkgs submission. But almost all packages from msstore source have agreements.

But since this manifest is coming from the context data, not being retrieved from a remote location, it isn't really a performance problem.

@yao-msft
Copy link
Contributor

This is less efficient if there is an agreement, but since most packages don't have agreements right now, it shouldn't (on average) change performance.

Actually, currently manifests with agreements will be blocked in winget-pkgs submission. But almost all packages from msstore source have agreements.

But since this manifest is coming from the context data, not being retrieved from a remote location, it isn't really a performance problem.

I agree. And actually we were already doing the same check several lines below. Just trying to clarify where agreements are being used.

@jedieaston
Copy link
Contributor Author

@JohnMcPMS Let me know which way you want to go on the table and I can make the adjustments. (I could go either way).

@JohnMcPMS
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@JohnMcPMS
Copy link
Member

@JohnMcPMS John McPherson FTE Let me know which way you want to go on the table and I can make the adjustments. (I could go either way).

I don't feel strongly about it really. It seems like the only strong opinion is to keep it, so I think at this point it is winning. The good thing is this is early in 1.3, so we can get feedback and update if needed,

@yao-msft yao-msft merged commit dc809d6 into microsoft:master Jan 22, 2022
@JohnLukeBentley
Copy link

@JohnMcPMS thanks for letting the table through (on the basis you mention). And @jedieaston thanks for the coding!

@denelon
Copy link
Contributor

denelon commented Jan 24, 2022

I know the PowerShell cmdlets are still super early. @jedieaston have you had a chance to see if this is breaking for anything that was hacked together there? I think the table output might be problematic for the way we're calling upgrade today.

@jedieaston
Copy link
Contributor Author

It looks like the Upgrade-WinGetPackage only upgrades a single package, so that isn't affected since this only shows the table on upgrade --all.

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.

winget upgrade --all. Insert blank line after intial "Found" list.
5 participants