Support notification on pwsh startup when a new release is available
#10689
Conversation
There was a problem hiding this comment.
Implementation looks really nice to me. Left a couple of comments but generally looks really good
src/Microsoft.PowerShell.ConsoleHost/host/msh/UpdatesNotification.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.PowerShell.ConsoleHost/host/msh/UpdatesNotification.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.PowerShell.ConsoleHost/host/msh/UpdatesNotification.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.PowerShell.ConsoleHost/host/msh/UpdatesNotification.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.PowerShell.ConsoleHost/host/msh/UpdatesNotification.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.PowerShell.ConsoleHost/host/msh/UpdatesNotification.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.PowerShell.ConsoleHost/host/msh/UpdatesNotification.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.PowerShell.ConsoleHost/host/msh/UpdatesNotification.cs
Outdated
Show resolved
Hide resolved
|
@rjmholt Thanks for the review and feedback! I addressed them all. Also, I decided to use |
src/Microsoft.PowerShell.ConsoleHost/host/msh/UpdatesNotification.cs
Outdated
Show resolved
Hide resolved
|
@joeyaiello Could you please take a look at the current notification message in the PR description? I decided to not print it in color as it's (1) over striking (annoying) (2) not accessibility friendly. |
src/Microsoft.PowerShell.ConsoleHost/host/msh/UpdatesNotification.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.PowerShell.ConsoleHost/host/msh/UpdatesNotification.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.PowerShell.ConsoleHost/host/msh/UpdatesNotification.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.PowerShell.ConsoleHost/host/msh/UpdatesNotification.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.PowerShell.ConsoleHost/host/msh/UpdatesNotification.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.PowerShell.ConsoleHost/host/msh/UpdatesNotification.cs
Outdated
Show resolved
Hide resolved
|
Rebased to resolve a conflict in experimental feature declaration. |
|
No tests was added. |
|
@iSazonov I don't know how to add a test for it. It seems manually testable only -- only interactive session will trigger update-check and notification-printing. |
|
We could use HttpListener helper module to emulate server responses, HelpersHostCS to get host output, and test hook to force update check. Although I'm not sure it is worth the effort. Maybe after it ceases to be experimental. |
|
@daxian-dbw |
|
@bergmeister I'm going to do an update to the message. Let me look at some examples from other software. |
|
@bergmeister This is an experimental feature and feedbacks is not a late. And we were more worried about the code than about the look. :-) |
Not clear how. |
|
@bergmeister Thanks for the feedback! I was expecting @joeyaiello to take a look at the message but he was busy and didn't get to it. |
PR Summary
RFC: PowerShell/PowerShell-RFC#162
Support notification on
pwshstartup when a new release is availablepwshwill check both new preview and stable releases. It prints notification message at startup time as long as the latest preview version or stable version is higher than the current version.pwshwill check new stable releases only. It prints notification message at startup time if the latest stable version is higher than the current version.-NoLogoflag and an environment variablePOWERSHELL_UPDATECHECK_OPTOUT.pwshstarts, so it has minimum impact to the startup time.For detailed implementation design, please look at the RFC.
Notification Message
For stable version
pwsh:For preview version
pwsh:Startup Perf Measurement on my dev machines
Windows
When no new release is available, namely on the latest preview/stable
pwshwhere notification will not be printed, the update notification change adds about4msto the startup time of an interactive session.2msspent on starting the update-check task2msspent on the notification printing logicWhen a new release is available, namely on an old preview/stable
pwshwhere a notification message will be printed, the update notification change adds about6msto the startup time of an interactive session.2msspent on starting the update-check task4msspent on the notification printing logicAs a reference,
Measure-Command { F:\pscore70.preview4\pwsh.exe -noprofile -c exit }takes about400mson average on my Windows dev machine.Linux (Ubuntu 16.04)
When no new release is available, namely on the latest preview/stable
pwshwhere notification will not be printed, the update notification change adds about6msto the startup time of an interactive session.3msspent on starting the update-check task3msspent on the notification printing logicWhen a new release is available, namely on an old preview/stable
pwshwhere a notification message will be printed, the update notification change adds about9msto the startup time of an interactive session.3msspent on starting the update-check task6msspent on the notification printing logicAs a reference,
Measure-Command { pwsh-preview -noprofile -c exit }takes about445mson average on my Linux dev machine.macOS (10.14.6)
When no new release is available, namely on the latest preview/stable
pwshwhere notification will not be printed, the update notification change adds about4msto the startup time of an interactive session.2msspent on starting the update-check task2msspent on the notification printing logicWhen a new release is available, namely on an old preview/stable
pwshwhere a notification message will be printed, the update notification change adds about5msto the startup time of an interactive session.2msspent on starting the update-check task3msspent on the notification printing logicAs a reference,
Measure-Command { pwsh-preview -noprofile -c exit }takes about316mson average on my macOS dev machine.PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.PSUpdatesNotification