-
Notifications
You must be signed in to change notification settings - Fork 7.3k
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
Support notification on pwsh
startup when a new release is available
#10689
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@rjmholt Thanks for the review and feedback! I addressed them all. Also, I decided to use |
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
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
pwsh
startup when a new release is availablepwsh
will 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.pwsh
will check new stable releases only. It prints notification message at startup time if the latest stable version is higher than the current version.-NoLogo
flag and an environment variablePOWERSHELL_UPDATECHECK_OPTOUT
.pwsh
starts, 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
pwsh
where notification will not be printed, the update notification change adds about4ms
to the startup time of an interactive session.2ms
spent on starting the update-check task2ms
spent on the notification printing logicWhen a new release is available, namely on an old preview/stable
pwsh
where a notification message will be printed, the update notification change adds about6ms
to the startup time of an interactive session.2ms
spent on starting the update-check task4ms
spent on the notification printing logicAs a reference,
Measure-Command { F:\pscore70.preview4\pwsh.exe -noprofile -c exit }
takes about400ms
on average on my Windows dev machine.Linux (Ubuntu 16.04)
When no new release is available, namely on the latest preview/stable
pwsh
where notification will not be printed, the update notification change adds about6ms
to the startup time of an interactive session.3ms
spent on starting the update-check task3ms
spent on the notification printing logicWhen a new release is available, namely on an old preview/stable
pwsh
where a notification message will be printed, the update notification change adds about9ms
to the startup time of an interactive session.3ms
spent on starting the update-check task6ms
spent on the notification printing logicAs a reference,
Measure-Command { pwsh-preview -noprofile -c exit }
takes about445ms
on average on my Linux dev machine.macOS (10.14.6)
When no new release is available, namely on the latest preview/stable
pwsh
where notification will not be printed, the update notification change adds about4ms
to the startup time of an interactive session.2ms
spent on starting the update-check task2ms
spent on the notification printing logicWhen a new release is available, namely on an old preview/stable
pwsh
where a notification message will be printed, the update notification change adds about5ms
to the startup time of an interactive session.2ms
spent on starting the update-check task3ms
spent on the notification printing logicAs a reference,
Measure-Command { pwsh-preview -noprofile -c exit }
takes about316ms
on average on my macOS dev machine.PR Checklist
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
or[ WIP ]
to the beginning of the title (theWIP
bot will keep its status check atPending
while the prefix is present) and remove the prefix when the PR is ready.PSUpdatesNotification