Extension Update Warning #6512
Open
Extension Update Warning #6512
+1,355
−18
Conversation
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
Collaborator
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsPowerShell install MSI install Standalone Binary
MSI
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
spboyer
approved these changes
Jan 14, 2026
wbreza
requested review from
JeffreyCA,
hemarina,
rajeshkamal5050,
tg-msft and
weikanglim
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds extension update checking functionality that notifies users when newer versions of installed extensions are available. The implementation runs update checks in parallel with extension execution to avoid adding latency.
Changes:
- Adds registry caching system with 4-hour TTL and per-source isolation
- Implements update checker with 24-hour warning cooldown tracked per extension
- Enhances WarningMessage UX component to support hint bullets
- Integrates update checks into extension command execution flow
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| cli/azd/pkg/output/ux/warning.go | Added Hints field to WarningMessage for displaying bulleted suggestions |
| cli/azd/pkg/extensions/extension.go | Added LastUpdateWarning field to track warning cooldown per extension |
| cli/azd/pkg/extensions/registry_cache.go | Implements per-source registry caching with TTL and file-based storage |
| cli/azd/pkg/extensions/registry_cache_test.go | Unit tests for cache manager functionality |
| cli/azd/pkg/extensions/update_checker.go | Core update checking logic with version comparison and cooldown management |
| cli/azd/pkg/extensions/update_checker_test.go | Unit tests for update checker |
| cli/azd/pkg/extensions/update_integration_test.go | Integration tests covering full update check workflow |
| cli/azd/pkg/extensions/manager.go | Added UpdateInstalled method to persist extension metadata changes |
| cli/azd/cmd/extensions.go | Integrated update checking into extension execution with async goroutine |
| cli/azd/.vscode/cspell.yaml | Added spelling exceptions for new terms |
Comments suppressed due to low confidence (4)
cli/azd/pkg/extensions/update_checker.go:92
- The comment uses "cool down" (two words) but elsewhere in the codebase it's written as "cooldown" (one word). Use consistent terminology throughout for better readability and searchability.
// ShouldShowWarning checks if a warning should be shown (respecting cool down)
cli/azd/cmd/extensions.go:172
- The defer function uses a non-blocking select with a default case to check if the update result is ready. If the goroutine hasn't completed by the time the extension finishes, the warning is silently skipped. This means fast-running extensions might never show update warnings even when updates are available. Consider either waiting for a reasonable timeout, or logging when the check didn't complete in time so users/developers are aware of the behavior.
defer func() {
// Collect result and show warning if needed (non-blocking read)
select {
case result := <-updateResultChan:
if result != nil && result.shouldShow && result.warning != nil {
a.console.MessageUxItem(ctx, result.warning)
a.console.Message(ctx, "")
}
default:
// Check didn't complete in time, skip warning
}
}()
cli/azd/pkg/extensions/manager.go:246
- The
UpdateInstalledmethod checks if the extension exists in the map but then overwrites it unconditionally. If the extension doesn't exist, the function returns an error, but if it does exist, the entire extension object is replaced rather than just updating specific fields. This means if the caller modifies the extension object between retrieval and update, unrelated fields could be changed. Consider documenting this full-replacement behavior, or implementing a more granular update mechanism.
// UpdateInstalled updates an installed extension's metadata in the config
func (m *Manager) UpdateInstalled(extension *Extension) error {
extensions, err := m.ListInstalled()
if err != nil {
return fmt.Errorf("failed to list installed extensions: %w", err)
}
if _, exists := extensions[extension.Id]; !exists {
return ErrInstalledExtensionNotFound
}
extensions[extension.Id] = extension
if err := m.userConfig.Set(installedConfigKey, extensions); err != nil {
return fmt.Errorf("failed to set extensions section: %w", err)
}
if err := m.configManager.Save(m.userConfig); err != nil {
return fmt.Errorf("failed to save user config: %w", err)
}
return nil
}
cli/azd/pkg/extensions/update_checker.go:37
- The comment uses "cool downs" (two words) but elsewhere in the codebase and in the constant name it's written as "cooldown" (one word). Use consistent terminology throughout for better readability and searchability.
// UpdateChecker checks for extension updates and manages warning cool downs
Comment on lines
+42
to
+46
| // NewUpdateChecker creates a new update checker | ||
| func NewUpdateChecker(cacheManager *RegistryCacheManager) (*UpdateChecker, error) { | ||
| return &UpdateChecker{ | ||
| cacheManager: cacheManager, | ||
| }, nil |
There was a problem hiding this comment.
The function NewUpdateChecker always returns a nil error. Consider either removing the error return value entirely and returning just *UpdateChecker, or document why an error return is kept for future extensibility. Keeping an unused error return can be confusing for callers.
| if err := m.configManager.Save(m.userConfig); err != nil { | ||
| return fmt.Errorf("failed to save user config: %w", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
After updating the extension in the map and saving the config, the Manager's cached m.installed map is not invalidated. This means if ListInstalled() was called before this update, subsequent calls to GetInstalled() or ListInstalled() will continue returning the old cached version until the Manager is recreated. Consider either invalidating the cache by setting m.installed = nil after the update, or updating the cached map directly to ensure consistency.
This issue also appears in the following locations of the same file:
- line 224
Suggested change
| // Invalidate the cached installed extensions so they are reloaded on next access. | |
| m.installed = nil |
| } | ||
|
|
||
| // RecordWarningShown updates the extension's LastUpdateWarning timestamp | ||
| // Returns the updated extension (caller should save it via Manager.UpdateInstalled) |
There was a problem hiding this comment.
The comment states "Returns the updated extension (caller should save it via Manager.UpdateInstalled)" but the function doesn't actually return anything. The function mutates the extension in place, which should be clearly documented. Update the comment to reflect that this function modifies the extension object directly rather than returning an updated copy.
Suggested change
| // Returns the updated extension (caller should save it via Manager.UpdateInstalled) | |
| // Mutates the provided extension in place (caller should save it via Manager.UpdateInstalled) |
| showUpdateWarning := !isJsonOutputFromArgs(os.Args) | ||
| if showUpdateWarning { | ||
| updateResultChan := make(chan *updateCheckOutcome, 1) | ||
| go a.checkForUpdateAsync(ctx, extension, updateResultChan) |
There was a problem hiding this comment.
The extension object is retrieved from the manager and then mutated in the goroutine by calling updateChecker.RecordWarningShown(extension) which modifies the LastUpdateWarning field. This creates a potential race condition if the extension object is shared or accessed elsewhere. Consider creating a copy of the extension before passing it to the goroutine, or use proper synchronization mechanisms.
Suggested change
| go a.checkForUpdateAsync(ctx, extension, updateResultChan) | |
| // Use a copy of the extension in the goroutine to avoid data races | |
| extCopy := *extension | |
| go a.checkForUpdateAsync(ctx, &extCopy, updateResultChan) |
Comment on lines
+18
to
+19
| // Hints are optional additional lines displayed as bullets below the warning | ||
| Hints []string |
There was a problem hiding this comment.
According to Go best practices and the coding guidelines requirement for comprehensive testing, the new WarningMessage.Hints functionality should have corresponding test coverage. There are no tests in the ux package that verify the ToString and MarshalJSON methods properly handle the Hints field, including rendering bullets, formatting, and JSON serialization.
Comment on lines
+28
to
+40
| // isJsonOutputFromArgs checks if --output json or -o json was passed in args | ||
| func isJsonOutputFromArgs(args []string) bool { | ||
| for i, arg := range args { | ||
| if arg == "--output" || arg == "-o" { | ||
| if i+1 < len(args) && args[i+1] == "json" { | ||
| return true | ||
| } | ||
| } | ||
| if arg == "--output=json" || arg == "-o=json" { | ||
| return true | ||
| } | ||
| } | ||
| return false |
There was a problem hiding this comment.
The function parses os.Args directly which includes all arguments passed to the azd CLI, not just the arguments for the extension command. This could incorrectly detect --output json if it appears in the parent command or in other unrelated contexts. Consider using the extension's specific args from a.args field instead, or ensuring the detection is scoped to the actual extension invocation arguments.
Comment on lines
+161
to
+172
| defer func() { | ||
| // Collect result and show warning if needed (non-blocking read) | ||
| select { | ||
| case result := <-updateResultChan: | ||
| if result != nil && result.shouldShow && result.warning != nil { | ||
| a.console.MessageUxItem(ctx, result.warning) | ||
| a.console.Message(ctx, "") | ||
| } | ||
| default: | ||
| // Check didn't complete in time, skip warning | ||
| } | ||
| }() |
There was a problem hiding this comment.
The deferred function that displays the warning will run before the function returns, but after any other deferred functions registered later in the function body. Since defer a.azdServer.Stop() is registered at line 204, the server will stop before the warning is shown. This ordering could be problematic if the warning display somehow depends on server availability, though in practice it likely won't matter. Consider adding a comment explaining the defer ordering or restructuring to make the execution order clearer.
This issue also appears in the following locations of the same file:
- line 161
| showUpdateWarning := !isJsonOutputFromArgs(os.Args) | ||
| if showUpdateWarning { | ||
| updateResultChan := make(chan *updateCheckOutcome, 1) | ||
| go a.checkForUpdateAsync(ctx, extension, updateResultChan) |
There was a problem hiding this comment.
The context passed to the goroutine originates from the parent Run method. If the parent context is cancelled after the goroutine starts but before update checking completes, operations in checkForUpdateAsync may fail or behave unexpectedly. Consider using context.WithoutCancel or creating a detached context for the background update check to ensure it can complete independently of the main command execution.
Suggested change
| go a.checkForUpdateAsync(ctx, extension, updateResultChan) | |
| go a.checkForUpdateAsync(context.WithoutCancel(ctx), extension, updateResultChan) |
Comment on lines
+91
to
+102
| // sanitizeSourceName converts a source name to a safe filename | ||
| func sanitizeSourceName(sourceName string) string { | ||
| // Replace unsafe characters with underscores | ||
| safe := sourceNameSanitizer.ReplaceAllString(sourceName, "_") | ||
| // Convert to lowercase for consistency | ||
| safe = strings.ToLower(safe) | ||
| // Ensure non-empty | ||
| if safe == "" { | ||
| safe = "default" | ||
| } | ||
| return safe | ||
| } |
There was a problem hiding this comment.
The source name sanitization converts to lowercase and replaces special characters with underscores, which could cause filename collisions for different source URLs. For example, "https://Example.com" and "https://example.com" would both sanitize to the same filename, as would "https://example.com/registry" and "https://example-com/registry". Consider using a hash of the original source name instead or appending a hash suffix to ensure uniqueness.
vhvb1989
approved these changes
Jan 15, 2026
JeffreyCA
reviewed
Jan 15, 2026
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Extension Update Warning
When users run an extension command, azd now checks if a newer version is available and displays a warning after execution with upgrade instructions.
Key Features
~/.azd/cache/extensions/<source>.jsonwith 4hr TTL (configurable viaAZD_EXTENSION_CACHE_TTL)Extension.LastUpdateWarningfield - auto-cleaned on uninstall--output jsonis usedExample Output