CLI: Make update notifications opt-in and suppress when --format json#17350
Merged
Conversation
Contributor
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 17350Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 17350" |
JamesNK
force-pushed
the
jamesn/update-notifications-opt-in
branch
from
c4a2075 to
0c50f27
Compare
JamesNK
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR changes Aspire CLI update-notification behavior to be opt-in per command, and adds suppression when JSON output is requested so machine-readable stdout isn’t polluted.
Changes:
- Flips the default for
BaseCommand.UpdateNotificationsEnabledso commands must explicitly opt in to show update notifications. - Suppresses update notifications when
--format jsonis requested. - Updates command implementations to opt in (
=> true) or remove now-redundant opt-out overrides, and adds a test for JSON suppression behavior.
Reviewed changes
Copilot reviewed 39 out of 39 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/Aspire.Cli.Tests/Commands/BaseCommandTests.cs | Adds a theory validating update-notification suppression for --format json. |
| src/Aspire.Cli/Commands/BaseCommand.cs | Changes default update-notification behavior and adds JSON suppression guard. |
| src/Aspire.Cli/Commands/AddCommand.cs | Explicitly opts into update notifications. |
| src/Aspire.Cli/Commands/DashboardRunCommand.cs | Explicitly opts into update notifications. |
| src/Aspire.Cli/Commands/DescribeCommand.cs | Explicitly opts into update notifications. |
| src/Aspire.Cli/Commands/ExportCommand.cs | Explicitly opts into update notifications. |
| src/Aspire.Cli/Commands/InitCommand.cs | Explicitly opts into update notifications. |
| src/Aspire.Cli/Commands/LogsCommand.cs | Explicitly opts into update notifications. |
| src/Aspire.Cli/Commands/LsCommand.cs | Explicitly opts into update notifications. |
| src/Aspire.Cli/Commands/NewCommand.cs | Explicitly opts into update notifications. |
| src/Aspire.Cli/Commands/PipelineCommandBase.cs | Explicitly opts into update notifications for pipeline-style commands. |
| src/Aspire.Cli/Commands/PsCommand.cs | Explicitly opts into update notifications. |
| src/Aspire.Cli/Commands/ResourceCommand.cs | Explicitly opts into update notifications. |
| src/Aspire.Cli/Commands/RestoreCommand.cs | Explicitly opts into update notifications. |
| src/Aspire.Cli/Commands/RunCommand.cs | Keeps conditional enablement (detach vs non-detach). |
| src/Aspire.Cli/Commands/SetupCommand.cs | Explicitly opts into update notifications. |
| src/Aspire.Cli/Commands/StartCommand.cs | Explicitly opts into update notifications. |
| src/Aspire.Cli/Commands/StopCommand.cs | Explicitly opts into update notifications. |
| src/Aspire.Cli/Commands/WaitCommand.cs | Explicitly opts into update notifications. |
| src/Aspire.Cli/Commands/UpdateCommand.cs | Removes explicit opt-out (now covered by default). |
| src/Aspire.Cli/Commands/Sdk/SdkDumpCommand.cs | Removes explicit opt-out (now covered by default). |
| src/Aspire.Cli/Commands/RenderCommand.cs | Removes explicit opt-out (now covered by default). |
| src/Aspire.Cli/Commands/ParentCommand.cs | Removes explicit opt-out (now covered by default). |
| src/Aspire.Cli/Commands/McpStartCommand.cs | Removes explicit opt-out (now covered by default). |
| src/Aspire.Cli/Commands/McpInitCommand.cs | Removes explicit opt-out (now covered by default). |
| src/Aspire.Cli/Commands/IntegrationSearchCommand.cs | Removes explicit opt-out (now covered by default). |
| src/Aspire.Cli/Commands/ExtensionInternalCommand.cs | Removes explicit opt-out (now covered by default). |
| src/Aspire.Cli/Commands/DoctorCommand.cs | Removes explicit opt-out (now covered by default). |
| src/Aspire.Cli/Commands/DocsSearchCommand.cs | Removes explicit opt-out (now covered by default). |
| src/Aspire.Cli/Commands/DocsListCommand.cs | Removes explicit opt-out (now covered by default). |
| src/Aspire.Cli/Commands/DocsGetCommand.cs | Removes explicit opt-out (now covered by default). |
| src/Aspire.Cli/Commands/ConfigCommand.cs | Removes explicit opt-out across config commands (now covered by default). |
| src/Aspire.Cli/Commands/CertificatesTrustCommand.cs | Removes explicit opt-out (now covered by default). |
| src/Aspire.Cli/Commands/CertificatesCleanCommand.cs | Removes explicit opt-out (now covered by default). |
| src/Aspire.Cli/Commands/CacheCommand.cs | Removes explicit opt-out (now covered by default). |
| src/Aspire.Cli/Commands/ApiSearchCommand.cs | Removes explicit opt-out (now covered by default). |
| src/Aspire.Cli/Commands/ApiListCommand.cs | Removes explicit opt-out (now covered by default). |
| src/Aspire.Cli/Commands/ApiGetCommand.cs | Removes explicit opt-out (now covered by default). |
| src/Aspire.Cli/Commands/AgentMcpCommand.cs | Removes explicit opt-out (now covered by default). |
| src/Aspire.Cli/Commands/AgentInitCommand.cs | Removes explicit opt-out (now covered by default). |
Comments suppressed due to low confidence (1)
src/Aspire.Cli/Commands/BaseCommand.cs:121
- The JSON-format suppression relies on IsJsonFormatRequested(parseResult), but that helper only recognizes an option typed as Option. At least one command (SecretListCommand) defines
--formatas Option<OutputFormat?>, so JSON won’t be detected there (meaning console redirection and this new update-notification suppression won’t apply even when--format jsonis used). Consider updating IsJsonFormatRequested to also handle Option<OutputFormat?> (and treat null as non-JSON).
if (UpdateNotificationsEnabled && !IsJsonFormatRequested(parseResult) && features.IsFeatureEnabled(KnownFeatures.UpdateNotificationsEnabled, true))
{
try
{
updateNotifier.NotifyIfUpdateAvailable();
Contributor
|
Re-running the failed jobs in the CI workflow for this pull request because 3 jobs were identified as retry-safe transient failures in the CI run attempt.
Matched test failure patterns (1 test)
|
Contributor
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
adamint
approved these changes
May 21, 2026
Member
adamint
left a comment
adamint
left a comment
There was a problem hiding this comment.
I'm l little unsure about showing this message for the commands I indicated.
| private static readonly int[] s_suppressErrorLogsMessageExitCodes = [CliExitCodes.Cancelled, CliExitCodes.MissingRequiredArgument]; | ||
|
|
||
| protected virtual bool UpdateNotificationsEnabled { get; } = true; | ||
| protected virtual bool UpdateNotificationsEnabled { get; } |
Member
There was a problem hiding this comment.
Since with Damian's changes we'll have two update messages, I think we should consider clarifying this member and renaming to UpdateAppHostNotificationsEnabled
| { | ||
| internal override HelpGroup HelpGroup => HelpGroup.Monitoring; | ||
|
|
||
| protected override bool UpdateNotificationsEnabled => true; |
Member
Author
There was a problem hiding this comment.
It was enabled before. A follow up PR can turn it off
| { | ||
| internal override HelpGroup HelpGroup => HelpGroup.Monitoring; | ||
|
|
||
| protected override bool UpdateNotificationsEnabled => true; |
Member
Author
There was a problem hiding this comment.
It was enabled before. A follow up PR can turn it off
| { | ||
| internal override HelpGroup HelpGroup => HelpGroup.Monitoring; | ||
|
|
||
| protected override bool UpdateNotificationsEnabled => true; |
Member
Author
There was a problem hiding this comment.
It was enabled before. A follow up PR can turn it off
| { | ||
| internal override HelpGroup HelpGroup => HelpGroup.AppCommands; | ||
|
|
||
| protected override bool UpdateNotificationsEnabled => true; |
Member
There was a problem hiding this comment.
I would only expect to see an aspire cli update msg, not apphost
Member
Author
There was a problem hiding this comment.
It was enabled before. A follow up PR can turn it off
| { | ||
| internal override HelpGroup HelpGroup => HelpGroup.ResourceManagement; | ||
|
|
||
| protected override bool UpdateNotificationsEnabled => true; |
Member
Author
There was a problem hiding this comment.
It was enabled before. A follow up PR can turn it off
| { | ||
| internal override HelpGroup HelpGroup => HelpGroup.ResourceManagement; | ||
|
|
||
| protected override bool UpdateNotificationsEnabled => true; |
Member
Author
There was a problem hiding this comment.
It was enabled before. A follow up PR can turn it off
Change BaseCommand.UpdateNotificationsEnabled default from true to false, so commands must explicitly opt-in to showing the update notification banner. Commands that opt-in are user-facing interactive commands where the notification is helpful. Commands that rely on the default (disabled) include machine-readable output commands, internal/utility commands, agent/MCP commands, and parent group commands where extra output would be noise.
Agent-Logs-Url: https://github.com/microsoft/aspire/sessions/8628fc02-4086-4c7e-aff1-67835431215e Co-authored-by: JamesNK <303201+JamesNK@users.noreply.github.com>
JamesNK
force-pushed
the
jamesn/update-notifications-opt-in
branch
from
54e32bd to
4c156c9
Compare
3 tasks
Update tests to use commands that have UpdateNotificationsEnabled set: - Use 'stop' (which has the override) instead of 'ps' for tests that assert notifications fire. - Use 'run --format json' instead of 'ps --format json' to properly test JSON format suppression (RunCommand has both --format and notifications enabled when not in detach mode).
Contributor
|
Re-running the failed jobs in the CI workflow for this pull request because 1 job was identified as retry-safe transient failures in the CI run attempt.
|
Contributor
|
❓ CLI E2E Tests unknown — 95 passed, 0 failed, 5 unknown (commit View all recordings
📹 Recordings uploaded automatically from CI run #26272065216 |
Contributor
|
✅ No documentation update needed. Documentation changes are needed for this PR ( Triggered signals: Proposed changes (ready to apply manually to
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two changes to update notification behavior in the CLI:
Default flipped to opt-in:
BaseCommand.UpdateNotificationsEnablednow defaults tofalse. Commands must explicitly override totrueto show the update notification banner. This prevents new commands from accidentally displaying notifications and ensures machine-readable output commands stay clean by default.Suppress when
--format json: Even if a command opts in to notifications, they are suppressed when the user requests JSON output (--format json), ensuring machine-parseable stdout is never polluted.Changes
BaseCommand.UpdateNotificationsEnableddefault changed fromtruetofalse!IsJsonFormatRequested(parseResult)guard to the notification check inBaseCommand=> trueto user-facing interactive commands=> falseoverridesCommands that display update notifications (
=> true)AddCommandDashboardRunCommandDescribeCommandExportCommandInitCommandLogsCommandLsCommandNewCommandPsCommandResourceCommandRestoreCommandRunCommandSetupCommandStartCommandStopCommandWaitCommandPipelineCommandBaseCommands that do not display update notifications (default)
AgentInitCommandAgentMcpCommandApiGetCommand,ApiListCommand,ApiSearchCommandCacheCommand.ClearCommandCertificatesCleanCommand,CertificatesTrustCommandConfigCommand+ subcommandsDoctorCommandDocsGetCommand,DocsListCommand,DocsSearchCommandExtensionInternalCommandIntegrationSearchCommand/IntegrationListCommandMcpCallCommand,McpInitCommand,McpStartCommand,McpToolsCommandParentCommandsubclassesRenderCommandRunCommand(detach mode)SdkDumpCommand,SdkGenerateCommandSecretDeleteCommand,SecretGetCommand,SecretListCommand,SecretPathCommand,SecretSetCommandTelemetryLogsCommand,TelemetrySpansCommand,TelemetryTracesCommandTemplateCommandUpdateCommand--format json