From 0498ab0ae050cb38d996635c19fe1be2c6ccc96a Mon Sep 17 00:00:00 2001 From: Matt Ellis Date: Mon, 9 Dec 2024 14:50:55 -0800 Subject: [PATCH] restore: Don't require `docker` for `restore` When running `azd restore` for a Docker based project, we were requiring that the docker tool itself was installed instead of whatever tool would be used by the actual source project. This has two implications: 1. If you didn't have the tool that was going to be invoked during `restore` installed (i.e. you didn't have `npm` for a node based project), we wouldn't detect the fact upfront and provide a nice error, we'd fail later when we tried to invoke the tool. 2. As of #4567, we now require that the docker daemon be running to consider docker "installed" (which is reasonable for cases where we are going to invoke docker commands) and so `azd restore` now fails if you have a docker based project but are not running the docker deamon (as is the case in the macOS environment in CI). To fix both of these issues, I decided to accept the fact that ensuring the tools needed to do a restore is different from the other cases, and so the `ProjectManager` now has a `EnsureRestoreTools` method. As part of this, I also changed the `ProjectManager` such that `Initialize` no longer calls `EnsureAllTools` (which is logically did before, it had just inlined the logic) and audited all the callers to ensure that they were calling `EnsureAllTools` themselves or another `EnsureXXXTools` variant (doing so also made me realize a lot of the care we had put into some of the commands controling what sets of tools they checked for via these special Ensure methods was being undone by the implict call to `EnsureAllTools` from `Initialize`. Fixes #4612 --- cli/azd/cmd/build.go | 8 +-- cli/azd/cmd/env.go | 4 ++ cli/azd/cmd/restore.go | 2 +- cli/azd/internal/cmd/provision.go | 4 ++ .../vsrpc/environment_service_refresh.go | 4 ++ cli/azd/pkg/project/project_manager.go | 59 ++++++++++++++----- cli/azd/test/functional/restore_test.go | 21 ------- cli/azd/test/functional/telemetry_test.go | 5 -- 8 files changed, 61 insertions(+), 46 deletions(-) diff --git a/cli/azd/cmd/build.go b/cli/azd/cmd/build.go index 7e1341900bf..853b4dfcdca 100644 --- a/cli/azd/cmd/build.go +++ b/cli/azd/cmd/build.go @@ -149,13 +149,13 @@ func (ba *buildAction) Run(ctx context.Context) (*actions.ActionResult, error) { return nil, err } - if err := ba.projectManager.EnsureFrameworkTools(ctx, ba.projectConfig, func(svc *project.ServiceConfig) bool { - return targetServiceName == "" || svc.Name == targetServiceName - }); err != nil { + if err := ba.projectManager.Initialize(ctx, ba.projectConfig); err != nil { return nil, err } - if err := ba.projectManager.Initialize(ctx, ba.projectConfig); err != nil { + if err := ba.projectManager.EnsureFrameworkTools(ctx, ba.projectConfig, func(svc *project.ServiceConfig) bool { + return targetServiceName == "" || svc.Name == targetServiceName + }); err != nil { return nil, err } diff --git a/cli/azd/cmd/env.go b/cli/azd/cmd/env.go index d9883e32d27..9e784106298 100644 --- a/cli/azd/cmd/env.go +++ b/cli/azd/cmd/env.go @@ -451,6 +451,10 @@ func (ef *envRefreshAction) Run(ctx context.Context) (*actions.ActionResult, err return nil, err } + if err := ef.projectManager.EnsureAllTools(ctx, ef.projectConfig, nil); err != nil { + return nil, err + } + infra, err := ef.importManager.ProjectInfrastructure(ctx, ef.projectConfig) if err != nil { return nil, err diff --git a/cli/azd/cmd/restore.go b/cli/azd/cmd/restore.go index 87d7b6ce3aa..b105a3c42e1 100644 --- a/cli/azd/cmd/restore.go +++ b/cli/azd/cmd/restore.go @@ -148,7 +148,7 @@ func (ra *restoreAction) Run(ctx context.Context) (*actions.ActionResult, error) return nil, err } - if err := ra.projectManager.EnsureFrameworkTools(ctx, ra.projectConfig, func(svc *project.ServiceConfig) bool { + if err := ra.projectManager.EnsureRestoreTools(ctx, ra.projectConfig, func(svc *project.ServiceConfig) bool { return targetServiceName == "" || svc.Name == targetServiceName }); err != nil { return nil, err diff --git a/cli/azd/internal/cmd/provision.go b/cli/azd/internal/cmd/provision.go index f1f829a881b..20ef1e110ab 100644 --- a/cli/azd/internal/cmd/provision.go +++ b/cli/azd/internal/cmd/provision.go @@ -185,6 +185,10 @@ func (p *ProvisionAction) Run(ctx context.Context) (*actions.ActionResult, error return nil, err } + if err := p.projectManager.EnsureAllTools(ctx, p.projectConfig, nil); err != nil { + return nil, err + } + infra, err := p.importManager.ProjectInfrastructure(ctx, p.projectConfig) if err != nil { return nil, err diff --git a/cli/azd/internal/vsrpc/environment_service_refresh.go b/cli/azd/internal/vsrpc/environment_service_refresh.go index d983920a562..d788d55a727 100644 --- a/cli/azd/internal/vsrpc/environment_service_refresh.go +++ b/cli/azd/internal/vsrpc/environment_service_refresh.go @@ -76,6 +76,10 @@ func (s *environmentService) refreshEnvironmentAsync( return nil, err } + if err := c.projectManager.EnsureAllTools(ctx, c.projectConfig, nil); err != nil { + return nil, err + } + infra, err := c.importManager.ProjectInfrastructure(ctx, c.projectConfig) if err != nil { return nil, err diff --git a/cli/azd/pkg/project/project_manager.go b/cli/azd/pkg/project/project_manager.go index eb59e858d7c..6dcf232a17a 100644 --- a/cli/azd/pkg/project/project_manager.go +++ b/cli/azd/pkg/project/project_manager.go @@ -37,8 +37,6 @@ type ProjectManager interface { // The initialization process will instantiate the framework & service target associated // with the service config that enables the scenario for these components to add event // handlers to participate in the lifecycle of an azd project - // - // The initialization process will also ensure that all required tools are installed Initialize(ctx context.Context, projectConfig *ProjectConfig) error // Returns the default service name to target based on the current working directory. @@ -58,6 +56,11 @@ type ProjectManager interface { // Ensures that all required service target tools are installed for the project and all child services EnsureServiceTargetTools(ctx context.Context, projectConfig *ProjectConfig, serviceFilterFn ServiceFilterPredicate) error + + // Ensures that all required tools for restore are installed for the project and all child services. This is like + // EnsureFrameworkTools but treats docker projects differently - it requires the tools for the inner project (i.e. npm) + // instead of needing docker tools themselves, since when doing a project restore, docker is not invoked. + EnsureRestoreTools(ctx context.Context, projectConfig *ProjectConfig, serviceFilterFn ServiceFilterPredicate) error } // ServiceFilterPredicate is a function that can be used to filter services that match a given criteria @@ -84,8 +87,6 @@ func NewProjectManager( // Initializes the project and all child services defined within the project configuration func (pm *projectManager) Initialize(ctx context.Context, projectConfig *ProjectConfig) error { - var projectTools []tools.ExternalTool - servicesStable, err := pm.importManager.ServiceStable(ctx, projectConfig) if err != nil { return err @@ -102,17 +103,6 @@ func (pm *projectManager) Initialize(ctx context.Context, projectConfig *Project if err := pm.serviceManager.Initialize(ctx, svc); err != nil { return fmt.Errorf("initializing service '%s', %w", svc.Name, err) } - - svcTools, err := pm.serviceManager.GetRequiredTools(ctx, svc) - if err != nil { - return fmt.Errorf("getting service required tools: %w", err) - } - - projectTools = append(projectTools, svcTools...) - } - - if err := tools.EnsureInstalled(ctx, tools.Unique(projectTools)...); err != nil { - return err } return nil @@ -243,3 +233,42 @@ func (pm *projectManager) EnsureServiceTargetTools( return nil } + +func (pm *projectManager) EnsureRestoreTools( + ctx context.Context, + projectConfig *ProjectConfig, + serviceFilterFn ServiceFilterPredicate, +) error { + var requiredTools []tools.ExternalTool + + servicesStable, err := pm.importManager.ServiceStable(ctx, projectConfig) + if err != nil { + return err + } + + for _, svc := range servicesStable { + if serviceFilterFn != nil && !serviceFilterFn(svc) { + continue + } + + frameworkService, err := pm.serviceManager.GetFrameworkService(ctx, svc) + if err != nil { + return fmt.Errorf("getting framework service: %w", err) + } + + var frameworkTools []tools.ExternalTool + if dp, ok := frameworkService.(*dockerProject); ok { + frameworkTools = dp.framework.RequiredExternalTools(ctx, svc) + } else { + frameworkTools = frameworkService.RequiredExternalTools(ctx, svc) + } + + requiredTools = append(requiredTools, frameworkTools...) + } + + if err := tools.EnsureInstalled(ctx, tools.Unique(requiredTools)...); err != nil { + return err + } + + return nil +} diff --git a/cli/azd/test/functional/restore_test.go b/cli/azd/test/functional/restore_test.go index 5bcb33b4443..7db4cdfc18d 100644 --- a/cli/azd/test/functional/restore_test.go +++ b/cli/azd/test/functional/restore_test.go @@ -4,7 +4,6 @@ import ( "fmt" "os" "path/filepath" - "runtime" "testing" "github.com/azure/azure-dev/cli/azd/pkg/environment/azdcontext" @@ -15,10 +14,6 @@ import ( // test for errors when running restore in invalid working directories func Test_CLI_Restore_Err_WorkingDirectory(t *testing.T) { - if runtime.GOOS == "darwin" { - t.Skip("azure/azure-dev#4612") - } - t.Parallel() ctx, cancel := newTestContext(t) defer cancel() @@ -84,10 +79,6 @@ func Test_CLI_Restore_Err_WorkingDirectory(t *testing.T) { // test restore in a service directory func Test_CLI_Restore_InServiceDirectory(t *testing.T) { - if runtime.GOOS == "darwin" { - t.Skip("azure/azure-dev#4612") - } - t.Parallel() ctx, cancel := newTestContext(t) defer cancel() @@ -120,10 +111,6 @@ func Test_CLI_Restore_InServiceDirectory(t *testing.T) { // test restore using a service name passed explicitly func Test_CLI_Restore_UsingServiceName(t *testing.T) { - if runtime.GOOS == "darwin" { - t.Skip("azure/azure-dev#4612") - } - t.Parallel() ctx, cancel := newTestContext(t) defer cancel() @@ -155,10 +142,6 @@ func Test_CLI_Restore_UsingServiceName(t *testing.T) { // test restore all in the project directory func Test_CLI_RestoreAll_InProjectDir(t *testing.T) { - if runtime.GOOS == "darwin" { - t.Skip("azure/azure-dev#4612") - } - t.Parallel() ctx, cancel := newTestContext(t) defer cancel() @@ -193,10 +176,6 @@ func Test_CLI_RestoreAll_InProjectDir(t *testing.T) { // test restore --all func Test_CLI_RestoreAll_UsingFlags(t *testing.T) { - if runtime.GOOS == "darwin" { - t.Skip("azure/azure-dev#4612") - } - // running this test in parallel is ok as it uses a t.TempDir() t.Parallel() ctx, cancel := newTestContext(t) diff --git a/cli/azd/test/functional/telemetry_test.go b/cli/azd/test/functional/telemetry_test.go index bf9710fd1db..388ad0106f7 100644 --- a/cli/azd/test/functional/telemetry_test.go +++ b/cli/azd/test/functional/telemetry_test.go @@ -12,7 +12,6 @@ import ( "os" "path/filepath" "regexp" - "runtime" "strings" "testing" @@ -134,10 +133,6 @@ func Test_CLI_Telemetry_UsageData_Simple_Command(t *testing.T) { // Verifies telemetry usage data generated when environments and projects are loaded. func Test_CLI_Telemetry_UsageData_EnvProjectLoad(t *testing.T) { - if runtime.GOOS == "darwin" { - t.Skip("azure/azure-dev#4612") - } - // CLI process and working directory are isolated ctx, cancel := newTestContext(t) defer cancel()