Skip to content

Commit bed176f

Browse files
spboyerCopilot
andcommitted
Refactor: use service target tool requirements instead of host assumptions
Per @vhvb1989 feedback: project_manager should not assume which targets support remoteBuild. Instead, check whether the service target's RequiredExternalTools actually listed Docker, which respects custom service targets from extensions. The suggestRemoteBuild helper now takes svcToolInfo (which services needed Docker per their target) instead of checking Host.RequiresContainer(). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 68de646 commit bed176f

2 files changed

Lines changed: 61 additions & 95 deletions

File tree

cli/azd/pkg/project/project_manager.go

Lines changed: 29 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -208,6 +208,12 @@ func (pm *projectManager) EnsureFrameworkTools(
208208
return nil
209209
}
210210

211+
// svcToolInfo tracks whether a service's target required Docker.
212+
type svcToolInfo struct {
213+
svc *ServiceConfig
214+
needsDocker bool
215+
}
216+
211217
func (pm *projectManager) EnsureServiceTargetTools(
212218
ctx context.Context,
213219
projectConfig *ProjectConfig,
@@ -220,6 +226,8 @@ func (pm *projectManager) EnsureServiceTargetTools(
220226
return err
221227
}
222228

229+
var svcTools []svcToolInfo
230+
223231
for _, svc := range servicesStable {
224232
if serviceFilterFn != nil && !serviceFilterFn(svc) {
225233
continue
@@ -230,13 +238,22 @@ func (pm *projectManager) EnsureServiceTargetTools(
230238
return fmt.Errorf("getting service target: %w", err)
231239
}
232240

233-
serviceTargetTools := serviceTarget.RequiredExternalTools(ctx, svc)
234-
requiredTools = append(requiredTools, serviceTargetTools...)
241+
targetTools := serviceTarget.RequiredExternalTools(ctx, svc)
242+
requiredTools = append(requiredTools, targetTools...)
243+
244+
needsDocker := false
245+
for _, tool := range targetTools {
246+
if tool.Name() == "Docker" {
247+
needsDocker = true
248+
break
249+
}
250+
}
251+
svcTools = append(svcTools, svcToolInfo{svc: svc, needsDocker: needsDocker})
235252
}
236253

237254
if err := tools.EnsureInstalled(ctx, tools.Unique(requiredTools)...); err != nil {
238255
if toolErr, ok := errors.AsType[*tools.MissingToolErrors](err); ok {
239-
if suggestion := suggestRemoteBuild(servicesStable, toolErr, serviceFilterFn); suggestion != nil {
256+
if suggestion := suggestRemoteBuild(svcTools, toolErr); suggestion != nil {
240257
return suggestion
241258
}
242259
}
@@ -246,35 +263,25 @@ func (pm *projectManager) EnsureServiceTargetTools(
246263
return nil
247264
}
248265

249-
// suggestRemoteBuild checks if Docker is in the missing tools list and whether the affected
250-
// services support remote builds. If so, it returns an ErrorWithSuggestion guiding the user
251-
// to enable remoteBuild instead of installing Docker locally.
266+
// suggestRemoteBuild checks if Docker is in the missing tools list and whether any
267+
// services that required it could use remote builds instead. Only services whose
268+
// service target actually listed Docker as required are included in the suggestion.
252269
func suggestRemoteBuild(
253-
services []*ServiceConfig,
270+
svcTools []svcToolInfo,
254271
toolErr *tools.MissingToolErrors,
255-
serviceFilterFn ServiceFilterPredicate,
256272
) *internal.ErrorWithSuggestion {
257273
if !slices.Contains(toolErr.ToolNames, "Docker") {
258274
return nil
259275
}
260276

261-
// Find services that actually require Docker/Podman locally and support remote builds.
262-
// Exclude services that use dotnet publish (no Docker needed) or already have remoteBuild enabled.
277+
// Find services that actually required Docker (per their service target)
278+
// and could use remoteBuild instead.
263279
var remoteBuildCapable []string
264-
for _, svc := range services {
265-
if serviceFilterFn != nil && !serviceFilterFn(svc) {
266-
continue
267-
}
268-
if !svc.Host.RequiresContainer() {
269-
continue
270-
}
271-
if svc.Docker.RemoteBuild {
272-
continue
273-
}
274-
if useDotnetPublishForDockerBuild(svc) {
280+
for _, info := range svcTools {
281+
if !info.needsDocker {
275282
continue
276283
}
277-
remoteBuildCapable = append(remoteBuildCapable, svc.Name)
284+
remoteBuildCapable = append(remoteBuildCapable, info.svc.Name)
278285
}
279286

280287
if len(remoteBuildCapable) == 0 {

cli/azd/pkg/project/project_manager_test.go

Lines changed: 32 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -27,128 +27,87 @@ func Test_suggestRemoteBuild(t *testing.T) {
2727
}
2828

2929
tests := []struct {
30-
name string
31-
services []*ServiceConfig
32-
toolErr *tools.MissingToolErrors
33-
serviceFilterFn ServiceFilterPredicate
34-
wantSuggestion bool
35-
wantContains string
30+
name string
31+
svcTools []svcToolInfo
32+
toolErr *tools.MissingToolErrors
33+
wantSuggestion bool
34+
wantContains string
3635
}{
3736
{
38-
name: "ContainerApp_without_remoteBuild_suggests",
39-
services: []*ServiceConfig{
40-
{Name: "api", Host: ContainerAppTarget},
37+
name: "Service_needing_Docker_suggests",
38+
svcTools: []svcToolInfo{
39+
{svc: &ServiceConfig{Name: "api"}, needsDocker: true},
4140
},
4241
toolErr: dockerMissing,
4342
wantSuggestion: true,
4443
wantContains: "api",
4544
},
46-
{
47-
name: "AKS_without_remoteBuild_suggests",
48-
services: []*ServiceConfig{
49-
{Name: "worker", Host: AksTarget},
50-
},
51-
toolErr: dockerMissing,
52-
wantSuggestion: true,
53-
wantContains: "worker",
54-
},
5545
{
5646
name: "Multiple_services_lists_all",
57-
services: []*ServiceConfig{
58-
{Name: "api", Host: ContainerAppTarget},
59-
{Name: "web", Host: ContainerAppTarget},
47+
svcTools: []svcToolInfo{
48+
{svc: &ServiceConfig{Name: "api"}, needsDocker: true},
49+
{svc: &ServiceConfig{Name: "web"}, needsDocker: true},
6050
},
6151
toolErr: dockerMissing,
6252
wantSuggestion: true,
6353
wantContains: "api, web",
6454
},
6555
{
66-
name: "ContainerApp_with_remoteBuild_no_suggestion",
67-
services: []*ServiceConfig{
68-
{Name: "api", Host: ContainerAppTarget, Docker: DockerProjectOptions{RemoteBuild: true}},
69-
},
70-
toolErr: dockerMissing,
71-
wantSuggestion: false,
72-
},
73-
{
74-
name: "AppService_no_suggestion",
75-
services: []*ServiceConfig{
76-
{Name: "web", Host: AppServiceTarget},
56+
name: "Service_not_needing_Docker_no_suggestion",
57+
svcTools: []svcToolInfo{
58+
{svc: &ServiceConfig{Name: "api"}, needsDocker: false},
7759
},
7860
toolErr: dockerMissing,
7961
wantSuggestion: false,
8062
},
8163
{
8264
name: "Non_Docker_tool_missing_no_suggestion",
83-
services: []*ServiceConfig{
84-
{Name: "api", Host: ContainerAppTarget},
65+
svcTools: []svcToolInfo{
66+
{svc: &ServiceConfig{Name: "api"}, needsDocker: true},
8567
},
8668
toolErr: bicepMissing,
8769
wantSuggestion: false,
8870
},
8971
{
90-
name: "Mixed_services_only_suggests_container_targets",
91-
services: []*ServiceConfig{
92-
{Name: "api", Host: ContainerAppTarget},
93-
{Name: "web", Host: AppServiceTarget},
94-
{Name: "worker", Host: AksTarget},
72+
name: "Mixed_services_only_Docker_ones",
73+
svcTools: []svcToolInfo{
74+
{svc: &ServiceConfig{Name: "api"}, needsDocker: true},
75+
{svc: &ServiceConfig{Name: "web"}, needsDocker: false},
76+
{svc: &ServiceConfig{Name: "worker"}, needsDocker: true},
9577
},
9678
toolErr: dockerMissing,
9779
wantSuggestion: true,
9880
wantContains: "api, worker",
9981
},
100-
{
101-
name: "Service_filter_excludes_service",
102-
services: []*ServiceConfig{
103-
{Name: "api", Host: ContainerAppTarget},
104-
{Name: "worker", Host: ContainerAppTarget},
105-
},
106-
toolErr: dockerMissing,
107-
serviceFilterFn: func(svc *ServiceConfig) bool {
108-
return svc.Name == "api"
109-
},
110-
wantSuggestion: true,
111-
wantContains: "api",
112-
},
113-
{
114-
name: "DotNet_without_Dockerfile_no_suggestion",
115-
services: func() []*ServiceConfig {
116-
useDotNet := true
117-
return []*ServiceConfig{
118-
{
119-
Name: "api",
120-
Host: ContainerAppTarget,
121-
Language: ServiceLanguageCsharp,
122-
useDotNetPublishForDockerBuild: &useDotNet,
123-
},
124-
}
125-
}(),
126-
toolErr: dockerMissing,
127-
wantSuggestion: false,
128-
},
12982
{
13083
name: "Docker_not_running_suggests_start",
131-
services: []*ServiceConfig{
132-
{Name: "api", Host: ContainerAppTarget},
84+
svcTools: []svcToolInfo{
85+
{svc: &ServiceConfig{Name: "api"}, needsDocker: true},
13386
},
13487
toolErr: dockerNotRunning,
13588
wantSuggestion: true,
13689
wantContains: "start your container runtime",
13790
},
13891
{
13992
name: "Docker_not_installed_suggests_install",
140-
services: []*ServiceConfig{
141-
{Name: "api", Host: ContainerAppTarget},
93+
svcTools: []svcToolInfo{
94+
{svc: &ServiceConfig{Name: "api"}, needsDocker: true},
14295
},
14396
toolErr: dockerMissing,
14497
wantSuggestion: true,
14598
wantContains: "install Docker",
14699
},
100+
{
101+
name: "Empty_services_no_suggestion",
102+
svcTools: []svcToolInfo{},
103+
toolErr: dockerMissing,
104+
wantSuggestion: false,
105+
},
147106
}
148107

149108
for _, tt := range tests {
150109
t.Run(tt.name, func(t *testing.T) {
151-
result := suggestRemoteBuild(tt.services, tt.toolErr, tt.serviceFilterFn)
110+
result := suggestRemoteBuild(tt.svcTools, tt.toolErr)
152111

153112
if !tt.wantSuggestion {
154113
assert.Nil(t, result)

0 commit comments

Comments
 (0)