Skip to content

Commit 56aa38f

Browse files
wbrezaCopilot
andcommitted
fix: address PR review feedback from @jongio
- Fix shell pipe handling in installer splitCommand for commands containing | or redirects (e.g., curl | sudo bash) - Remove accidentally committed test coverage file, add to .gitignore - Change MCP server detection from npx (triggers download) to npm list -g (local check only) - Move upgrade execution inside TaskList callbacks so spinners reflect real-time progress - Replace hardcoded workflow command allowlist with group annotation check (CmdGroupStart/CmdGroupAzure/CmdGroupBeta) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent b382060 commit 56aa38f

File tree

6 files changed

+103
-364
lines changed

6 files changed

+103
-364
lines changed

cli/azd/.gitignore

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ azd.sln
1010
**/target
1111

1212
# Coverage artifacts (generated by mage coverage:* targets)
13+
cover
1314
cover-local.out
1415
cover-ci-combined.out
1516
coverage.html

cli/azd/cmd/root.go

Lines changed: 18 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -282,6 +282,9 @@ func newRootCmd(
282282
ActionResolver: newBuildAction,
283283
OutputFormats: []output.Format{output.JsonFormat, output.NoneFormat},
284284
DefaultFormat: output.NoneFormat,
285+
GroupingOptions: actions.CommandGroupOptions{
286+
RootLevelHelp: actions.CmdGroupBeta,
287+
},
285288
}).
286289
UseMiddleware("hooks", middleware.NewHooksMiddleware).
287290
UseMiddleware("extensions", middleware.NewExtensionsMiddleware)
@@ -547,27 +550,25 @@ func newRootCmd(
547550
// command where a first-run tool check adds value. Utility commands
548551
// (auth, config, env, show, etc.) are excluded so they are never
549552
// blocked by the first-run experience or update notifications.
553+
//
554+
// Instead of maintaining a hard-coded allowlist, we rely on the
555+
// RootLevelHelpOption group annotation so that any new command added
556+
// to a workflow group is automatically covered.
550557
func isWorkflowCommand(descriptor *actions.ActionDescriptor) bool {
551-
workflowCommands := map[string]bool{
552-
"init": true,
553-
"up": true,
554-
"deploy": true,
555-
"provision": true,
556-
"build": true,
557-
"package": true,
558-
"restore": true,
559-
"publish": true,
560-
"down": true,
561-
"monitor": true,
562-
"add": true,
558+
// Walk up to the root-level subcommand (first segment after "azd").
559+
current := descriptor
560+
for current.Parent() != nil && current.Parent().Parent() != nil {
561+
current = current.Parent()
563562
}
564563

565-
// Walk up to the root-level subcommand (first segment after "azd").
566-
cmd := descriptor.Options.Command
567-
for cmd.Parent() != nil && cmd.Parent().Parent() != nil {
568-
cmd = cmd.Parent()
564+
if current.Options == nil {
565+
return false
569566
}
570-
return workflowCommands[cmd.Name()]
567+
568+
group := current.Options.GroupingOptions.RootLevelHelp
569+
return group == actions.CmdGroupStart ||
570+
group == actions.CmdGroupAzure ||
571+
group == actions.CmdGroupBeta
571572
}
572573

573574
func getCmdRootHelpFooter(cmd *cobra.Command) string {

cli/azd/cmd/tool.go

Lines changed: 38 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -516,20 +516,32 @@ func (a *toolUpgradeAction) Run(ctx context.Context) (*actions.ActionResult, err
516516
TitleNote: "Upgrades installed tools to their latest versions",
517517
})
518518

519-
var results []*tool.InstallResult
520-
var err error
519+
// Determine which tools to upgrade — resolve tool definitions
520+
// up front but defer the actual upgrade work to each task callback
521+
// so that the spinner reflects real-time progress.
522+
var toolsToUpgrade []*tool.ToolDefinition
521523

522524
if len(a.args) > 0 {
523-
results, err = a.manager.UpgradeTools(ctx, a.args)
525+
for _, id := range a.args {
526+
toolDef, findErr := a.manager.FindTool(id)
527+
if findErr != nil {
528+
return nil, findErr
529+
}
530+
toolsToUpgrade = append(toolsToUpgrade, toolDef)
531+
}
524532
} else {
525-
results, err = a.manager.UpgradeAll(ctx)
526-
}
527-
528-
if err != nil {
529-
return nil, fmt.Errorf("upgrading tools: %w", err)
533+
statuses, detectErr := a.manager.DetectAll(ctx)
534+
if detectErr != nil {
535+
return nil, fmt.Errorf("detecting installed tools: %w", detectErr)
536+
}
537+
for _, s := range statuses {
538+
if s.Installed {
539+
toolsToUpgrade = append(toolsToUpgrade, s.Tool)
540+
}
541+
}
530542
}
531543

532-
if len(results) == 0 {
544+
if len(toolsToUpgrade) == 0 {
533545
a.console.Message(ctx, output.WithGrayFormat("No installed tools to upgrade."))
534546
return nil, nil
535547
}
@@ -538,19 +550,26 @@ func (a *toolUpgradeAction) Run(ctx context.Context) (*actions.ActionResult, err
538550
&uxlib.TaskListOptions{ContinueOnError: true},
539551
)
540552

541-
for _, r := range results {
542-
capturedResult := r
553+
for _, t := range toolsToUpgrade {
554+
capturedTool := t
543555
taskList.AddTask(uxlib.TaskOptions{
544-
Title: fmt.Sprintf("Upgrading %s", capturedResult.Tool.Name),
556+
Title: fmt.Sprintf("Upgrading %s", capturedTool.Name),
545557
Action: func(setProgress uxlib.SetProgressFunc) (uxlib.TaskState, error) {
546-
if capturedResult.Error != nil {
547-
return uxlib.Error, capturedResult.Error
548-
}
549-
if !capturedResult.Success {
550-
return uxlib.Warning, fmt.Errorf("upgrade did not succeed")
558+
results, upgradeErr := a.manager.UpgradeTools(ctx, []string{capturedTool.Id})
559+
if upgradeErr != nil {
560+
return uxlib.Error, upgradeErr
551561
}
552-
if capturedResult.InstalledVersion != "" {
553-
setProgress(capturedResult.InstalledVersion)
562+
if len(results) > 0 {
563+
r := results[0]
564+
if r.Error != nil {
565+
return uxlib.Error, r.Error
566+
}
567+
if !r.Success {
568+
return uxlib.Warning, fmt.Errorf("upgrade did not succeed")
569+
}
570+
if r.InstalledVersion != "" {
571+
setProgress(r.InstalledVersion)
572+
}
554573
}
555574
return uxlib.Success, nil
556575
},

0 commit comments

Comments
 (0)