diff --git a/cli/azd/cmd/hooks.go b/cli/azd/cmd/hooks.go index 1194b92be2f..15ef0b858f6 100644 --- a/cli/azd/cmd/hooks.go +++ b/cli/azd/cmd/hooks.go @@ -129,6 +129,11 @@ func (hra *hooksRunAction) Run(ctx context.Context) (*actions.ActionResult, erro ), }) + // Validate hooks and display warnings + if err := hra.validateAndWarnHooks(ctx); err != nil { + return nil, fmt.Errorf("failed validating hooks: %w", err) + } + // Validate service name if hra.flags.service != "" { if has, err := hra.importManager.HasService(ctx, hra.projectConfig, hra.flags.service); err != nil { @@ -264,6 +269,44 @@ func (hra *hooksRunAction) execHook( return nil } +// Validates hooks and displays warnings for default shell usage and other issues +func (hra *hooksRunAction) validateAndWarnHooks(ctx context.Context) error { + // Collect all hooks from project and services + allHooks := make(map[string][]*ext.HookConfig) + + // Add project hooks + for hookName, hookConfigs := range hra.projectConfig.Hooks { + allHooks[hookName] = append(allHooks[hookName], hookConfigs...) + } + + // Add service hooks + stableServices, err := hra.importManager.ServiceStable(ctx, hra.projectConfig) + if err == nil { + for _, service := range stableServices { + for hookName, hookConfigs := range service.Hooks { + allHooks[hookName] = append(allHooks[hookName], hookConfigs...) + } + } + } + + // Create hooks manager and validate + hooksManager := ext.NewHooksManager(hra.projectConfig.Path, hra.commandRunner) + validationResult := hooksManager.ValidateHooks(ctx, allHooks) + + // Display any warnings + for _, warning := range validationResult.Warnings { + hra.console.MessageUxItem(ctx, &ux.WarningMessage{ + Description: warning.Message, + }) + if warning.Suggestion != "" { + hra.console.Message(ctx, warning.Suggestion) + } + hra.console.Message(ctx, "") + } + + return nil +} + // Overrides the configured hooks from command line flags func (hra *hooksRunAction) prepareHook(name string, hook *ext.HookConfig) error { // Enable testing cross platform diff --git a/cli/azd/pkg/ext/hooks_manager.go b/cli/azd/pkg/ext/hooks_manager.go index fc81cb64a87..17c02f452ce 100644 --- a/cli/azd/pkg/ext/hooks_manager.go +++ b/cli/azd/pkg/ext/hooks_manager.go @@ -10,6 +10,7 @@ import ( "log" "os" osexec "os/exec" + "path/filepath" "runtime" "strings" @@ -142,17 +143,57 @@ func (h *HooksManager) ValidateHooks(ctx context.Context, allHooks map[string][] } hasPowerShellHooks := false + hasDefaultShellHooks := false - // Check all hooks + // Two-pass validation is required because: + // 1. First pass: Set shell defaults and detect inline scripts for each hook configuration + // 2. Second pass: Generate warnings only after all hooks have been processed and we know + // the complete state (e.g., whether ANY hook uses PowerShell or default shell) + // We cannot merge these loops because warnings depend on global state across all hooks. + + // First pass: perform lightweight validation to set flags like usingDefaultShell + // without creating temporary files (which full validation does) + for _, hookConfigs := range allHooks { + for _, hookConfig := range hookConfigs { + // Set the working directory for validation + if hookConfig.cwd == "" { + hookConfig.cwd = h.cwd + } + + // Only perform shell detection for warning purposes, not full validation + if !hookConfig.validated && hookConfig.Run != "" { + // Check if it's an inline script (no file exists) + relativeCheckPath := strings.ReplaceAll(hookConfig.Run, "/", string(os.PathSeparator)) + fullCheckPath := relativeCheckPath + if hookConfig.cwd != "" { + fullCheckPath = filepath.Join(hookConfig.cwd, hookConfig.Run) + } + + _, err := os.Stat(fullCheckPath) + isInlineScript := err != nil // File doesn't exist, so it's inline + + // If shell is not specified and it's an inline script, set default for warning + if hookConfig.Shell == ScriptTypeUnknown && isInlineScript { + if runtime.GOOS == "windows" { + hookConfig.Shell = ShellTypePowershell + } else { + hookConfig.Shell = ShellTypeBash + } + hookConfig.usingDefaultShell = true + } + } + } + } + + // Second pass: check all hooks for warning conditions using the state set in first pass for _, hookConfigs := range allHooks { for _, hookConfig := range hookConfigs { if hookConfig.IsPowerShellHook() { hasPowerShellHooks = true - break } - } - if hasPowerShellHooks { - break + if hookConfig.IsUsingDefaultShell() { + hasDefaultShellHooks = true + } } } @@ -183,5 +224,33 @@ func (h *HooksManager) ValidateHooks(ctx context.Context, allHooks map[string][] } } + // If we found hooks using default shell, warn the user + if hasDefaultShellHooks { + var warningMessage string + var defaultShell string + + if runtime.GOOS == "windows" { + defaultShell = "pwsh" + } else { + defaultShell = "sh" + } + + warningMessage = fmt.Sprintf( + "Hook configurations found without explicit shell specification. Using OS default shell '%s'. "+ + "For better reliability, consider specifying the shell explicitly in your hook configuration.\n"+ + "More about using hooks: %s", + defaultShell, + output.WithHyperlink("aka.ms/azd-hooks", "aka.ms/azd-hooks"), + ) + + result.Warnings = append(result.Warnings, HookWarning{ + Message: warningMessage, + Suggestion: fmt.Sprintf( + "Add 'shell: %s' to your hook configurations to remove this warning.", + defaultShell, + ), + }) + } + return result } diff --git a/cli/azd/pkg/ext/hooks_manager_test.go b/cli/azd/pkg/ext/hooks_manager_test.go index d567abab70a..e72c7940ac1 100644 --- a/cli/azd/pkg/ext/hooks_manager_test.go +++ b/cli/azd/pkg/ext/hooks_manager_test.go @@ -4,6 +4,7 @@ package ext import ( + "context" "os" "path/filepath" "regexp" @@ -11,6 +12,7 @@ import ( "testing" "github.com/azure/azure-dev/cli/azd/pkg/osutil" + "github.com/azure/azure-dev/cli/azd/test/mocks" "github.com/azure/azure-dev/cli/azd/test/mocks/mockexec" "github.com/azure/azure-dev/cli/azd/test/ostest" "github.com/stretchr/testify/require" @@ -45,16 +47,18 @@ func Test_GetAllHookConfigs(t *testing.T) { }) t.Run("With Invalid Configuration", func(t *testing.T) { - // All hooksMap are invalid because they are missing a script type + // All hooksMap are invalid because they are missing the run parameter hooksMap := map[string][]*HookConfig{ "preinit": { { - Run: "echo 'Hello'", + Shell: ShellTypeBash, + // Run is missing - this should cause an error }, }, "postinit": { { - Run: "echo 'Hello'", + Shell: ShellTypeBash, + // Run is missing - this should cause an error }, }, } @@ -115,16 +119,18 @@ func Test_GetByParams(t *testing.T) { }) t.Run("With Invalid Configuration", func(t *testing.T) { - // All hooksMap are invalid because they are missing a script type + // All hooksMap are invalid because they are missing the run parameter hooksMap := map[string][]*HookConfig{ "preinit": { { - Run: "echo 'Hello'", + Shell: ShellTypeBash, + // Run is missing - this should cause an error }, }, "postinit": { { - Run: "echo 'Hello'", + Shell: ShellTypeBash, + // Run is missing - this should cause an error }, }, } @@ -140,6 +146,88 @@ func Test_GetByParams(t *testing.T) { }) } +func Test_HookConfig_DefaultShell(t *testing.T) { + tests := []struct { + name string + hookConfig *HookConfig + expectedShell ShellType + expectingDefault bool + }{ + { + name: "No shell specified - should use OS default", + hookConfig: &HookConfig{ + Name: "test", + Run: "echo 'hello'", + }, + expectedShell: getDefaultShellForOS(), + expectingDefault: true, + }, + { + name: "Shell explicitly specified - should not use default", + hookConfig: &HookConfig{ + Name: "test", + Shell: ShellTypeBash, + Run: "echo 'hello'", + }, + expectedShell: ShellTypeBash, + expectingDefault: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Clone the config to avoid modifying the test case + config := *tt.hookConfig + config.cwd = t.TempDir() + + err := config.validate() + require.NoError(t, err) + + require.Equal(t, tt.expectedShell, config.Shell) + require.Equal(t, tt.expectingDefault, config.IsUsingDefaultShell()) + }) + } +} + +func Test_HooksManager_ValidateDefaultShellWarning(t *testing.T) { + tempDir := t.TempDir() + mockContext := mocks.NewMockContext(context.Background()) + hooksManager := NewHooksManager(tempDir, mockContext.CommandRunner) + + // Create hooks without explicit shell configuration + // DON'T pre-validate - let ValidateHooks do the validation and warning detection + hooksWithoutShell := map[string][]*HookConfig{ + "prebuild": { + { + Name: "prebuild", + Run: "echo 'Building...'", + // No Shell specified - should trigger default shell warning + // No cwd specified - ValidateHooks should set it + }, + }, + } + + // ValidateHooks should validate the hooks internally and detect default shell usage + result := hooksManager.ValidateHooks(context.Background(), hooksWithoutShell) + + // Should have at least one warning about default shell usage + hasDefaultShellWarning := false + for _, warning := range result.Warnings { + if strings.Contains(warning.Message, "Hook configurations found without explicit shell specification") { + hasDefaultShellWarning = true + break + } + } + + require.True(t, hasDefaultShellWarning, "Expected warning about default shell usage") + + // Also verify that the hook was actually validated and has the default shell set + hook := hooksWithoutShell["prebuild"][0] + require.True(t, hook.IsUsingDefaultShell(), "Hook should be marked as using default shell") + expectedShell := getDefaultShellForOS() + require.Equal(t, expectedShell, hook.Shell, "Hook should have the OS default shell") +} + func ensureScriptsExist(t *testing.T, configs map[string][]*HookConfig) { for _, hooks := range configs { for _, hook := range hooks { diff --git a/cli/azd/pkg/ext/hooks_runner_test.go b/cli/azd/pkg/ext/hooks_runner_test.go index f838253853a..3db41883bb6 100644 --- a/cli/azd/pkg/ext/hooks_runner_test.go +++ b/cli/azd/pkg/ext/hooks_runner_test.go @@ -450,12 +450,12 @@ func Test_GetScript_Validation(t *testing.T) { scriptValidations := []scriptValidationTest{ { - name: "Missing Script Type", + name: "Missing Script Type - Should Use Default Shell", config: &HookConfig{ Name: "test1", Run: "echo 'Hello'", }, - expectedError: ErrScriptTypeUnknown, + expectedError: nil, // Should no longer error, should use default shell }, { name: "Missing Run param", diff --git a/cli/azd/pkg/ext/models.go b/cli/azd/pkg/ext/models.go index 307a08e1b49..91a12b96495 100644 --- a/cli/azd/pkg/ext/models.go +++ b/cli/azd/pkg/ext/models.go @@ -59,6 +59,8 @@ type HookConfig struct { cwd string // When location is `inline` a script must be defined inline script string + // Indicates if the shell was automatically detected based on OS (used for warnings) + usingDefaultShell bool // Internal name of the hook running for a given command Name string `yaml:",omitempty"` @@ -104,8 +106,10 @@ func (hc *HookConfig) validate() error { hc.script = hc.Run } + // If shell is not specified and it's an inline script, use OS default if hc.Shell == ScriptTypeUnknown && hc.path == "" { - return ErrScriptTypeUnknown + hc.Shell = getDefaultShellForOS() + hc.usingDefaultShell = true } if hc.location == ScriptLocationUnknown { @@ -164,6 +168,12 @@ func (hc *HookConfig) IsPowerShellHook() bool { return false } +// IsUsingDefaultShell returns true if the hook is using the OS default shell +// because no shell was explicitly configured +func (hc *HookConfig) IsUsingDefaultShell() bool { + return hc.usingDefaultShell +} + func InferHookType(name string) (HookType, string) { // Validate name length so go doesn't PANIC for string slicing below if len(name) < 4 { @@ -177,6 +187,14 @@ func InferHookType(name string) (HookType, string) { return HookTypeNone, name } +// getDefaultShellForOS returns the default shell type based on the operating system +func getDefaultShellForOS() ShellType { + if runtime.GOOS == "windows" { + return ShellTypePowershell + } + return ShellTypeBash +} + func inferScriptTypeFromFilePath(path string) (ShellType, error) { fileExtension := filepath.Ext(path) switch fileExtension {