Allow configuration to fail when a hook execution fails, fixes #2536 (#…

…2652)

cmd/ddev/cmd/config-global.go

@@ -86,6 +86,12 @@ func handleGlobalConfig(cmd *cobra.Command, args []string) {
 		dirty = true
 	}
 
+	if cmd.Flag("fail-on-hook-fail").Changed {
+		val, _ := cmd.Flags().GetBool("fail-on-hook-fail")
+		globalconfig.DdevGlobalConfig.FailOnHookFailGlobal = val
+		dirty = true
+	}
+
 	if dirty {
 		err = globalconfig.ValidateGlobalConfig()
 		if err != nil {
@@ -107,6 +113,7 @@ func handleGlobalConfig(cmd *cobra.Command, args []string) {
 	output.UserOut.Printf("letsencrypt-email=%v", globalconfig.DdevGlobalConfig.LetsEncryptEmail)
 	output.UserOut.Printf("auto-restart-containers=%v", globalconfig.DdevGlobalConfig.AutoRestartContainers)
 	output.UserOut.Printf("use-hardened-images=%v", globalconfig.DdevGlobalConfig.UseHardenedImages)
+	output.UserOut.Printf("fail-on-hook-fail=%v", globalconfig.DdevGlobalConfig.FailOnHookFailGlobal)
 }
 
 func init() {
@@ -119,6 +126,7 @@ func init() {
 	configGlobalCommand.Flags().String("letsencrypt-email", "", "Email associated with Let's Encrypt, `ddev global --letsencrypt-email=me@example.com'")
 	configGlobalCommand.Flags().Bool("auto-restart-containers", false, "If true, automatically restart containers after a reboot or docker restart")
 	configGlobalCommand.Flags().Bool("use-hardened-images", false, "If true, use more secure 'hardened' images for an actual internet deployment.")
+	configGlobalCommand.Flags().Bool("fail-on-hook-fail", false, "If true, 'ddev start' will fail when a hook fails.")
 
 	ConfigCommand.AddCommand(configGlobalCommand)
 }

cmd/ddev/cmd/config-global_test.go

@@ -45,13 +45,13 @@ func TestCmdGlobalConfig(t *testing.T) {
 	args := []string{"config", "global"}
 	out, err := exec.RunCommand(DdevBin, args)
 	assert.NoError(err)
-	assert.Contains(string(out), "Global configuration:\ninstrumentation-opt-in=false\nomit-containers=[]\nnfs-mount-enabled=false\nrouter-bind-all-interfaces=false\ninternet-detection-timeout-ms=750\nuse-letsencrypt=false\nletsencrypt-email=\nauto-restart-containers=false\nuse-hardened-images=false")
+	assert.Contains(string(out), "Global configuration:\ninstrumentation-opt-in=false\nomit-containers=[]\nnfs-mount-enabled=false\nrouter-bind-all-interfaces=false\ninternet-detection-timeout-ms=750\nuse-letsencrypt=false\nletsencrypt-email=\nauto-restart-containers=false\nuse-hardened-images=false\nfail-on-hook-fail=false")
 
 	// Update a config
-	args = []string{"config", "global", "--instrumentation-opt-in=false", "--omit-containers=dba,ddev-ssh-agent", "--nfs-mount-enabled=true", "--router-bind-all-interfaces=true", "--internet-detection-timeout-ms=850", "--use-letsencrypt", "--letsencrypt-email=nobody@example.com", "--auto-restart-containers=true", "--use-hardened-images=true"}
+	args = []string{"config", "global", "--instrumentation-opt-in=false", "--omit-containers=dba,ddev-ssh-agent", "--nfs-mount-enabled=true", "--router-bind-all-interfaces=true", "--internet-detection-timeout-ms=850", "--use-letsencrypt", "--letsencrypt-email=nobody@example.com", "--auto-restart-containers=true", "--use-hardened-images=true", "--fail-on-hook-fail=true"}
 	out, err = exec.RunCommand(DdevBin, args)
 	assert.NoError(err)
-	assert.Contains(string(out), "Global configuration:\ninstrumentation-opt-in=false\nomit-containers=[dba,ddev-ssh-agent]\nnfs-mount-enabled=true\nrouter-bind-all-interfaces=true\ninternet-detection-timeout-ms=850\nuse-letsencrypt=true\nletsencrypt-email=nobody@example.com\nauto-restart-containers=true\nuse-hardened-images=true")
+	assert.Contains(string(out), "Global configuration:\ninstrumentation-opt-in=false\nomit-containers=[dba,ddev-ssh-agent]\nnfs-mount-enabled=true\nrouter-bind-all-interfaces=true\ninternet-detection-timeout-ms=850\nuse-letsencrypt=true\nletsencrypt-email=nobody@example.com\nauto-restart-containers=true\nuse-hardened-images=true\nfail-on-hook-fail=true")
 
 	err = globalconfig.ReadGlobalConfig()
 	assert.NoError(err)
@@ -63,6 +63,7 @@ func TestCmdGlobalConfig(t *testing.T) {
 	assert.Equal("nobody@example.com", globalconfig.DdevGlobalConfig.LetsEncryptEmail)
 	assert.True(globalconfig.DdevGlobalConfig.UseLetsEncrypt)
 	assert.True(globalconfig.DdevGlobalConfig.UseHardenedImages)
+	assert.True(globalconfig.DdevGlobalConfig.FailOnHookFailGlobal)
 
 	// Even though the global config is going to be deleted, make sure it's sane before leaving
 	args = []string{"config", "global", "--omit-containers", "", "--nfs-mount-enabled=true"}

cmd/ddev/cmd/config.go

@@ -111,6 +111,9 @@ var (
 	// nfsMountEnabled sets nfs_mount_enabled
 	nfsMountEnabled bool
 
+	// failOnHookFail sets fail_on_hook_fail
+	failOnHookFail bool
+
 	// hostDBPortArg sets host_db_port
 	hostDBPortArg string
 
@@ -267,6 +270,7 @@ func init() {
 	ConfigCommand.Flags().String("mysql-version", "", "Oracle mysql version to use (incompatible with --mariadb-version)")
 
 	ConfigCommand.Flags().BoolVar(&nfsMountEnabled, "nfs-mount-enabled", false, "enable NFS mounting of project in container")
+	ConfigCommand.Flags().BoolVar(&failOnHookFail, "fail-on-hook-fail", false, "Decide whether 'ddev start' should be interrupted by a failing hook")
 	ConfigCommand.Flags().StringVar(&hostWebserverPortArg, "host-webserver-port", "", "The web container's localhost-bound port")
 	ConfigCommand.Flags().StringVar(&hostHTTPSPortArg, "host-https-port", "", "The web container's localhost-bound https port")
 
@@ -479,6 +483,10 @@ func handleMainConfigArgs(cmd *cobra.Command, args []string, app *ddevapp.DdevAp
 		app.NFSMountEnabled = nfsMountEnabled
 	}
 
+	if cmd.Flag("fail-on-hook-fail").Changed {
+		app.FailOnHookFail = failOnHookFail
+	}
+
 	// This bool flag is false by default, so only use the value if the flag was explicity set.
 	if cmd.Flag("xdebug-enabled").Changed {
 		app.XdebugEnabled = xdebugEnabledArg

docs/users/extend/config_yaml.md

@@ -30,6 +30,7 @@ the .ddev/config.yaml is the primary configuration for the project.
 | working_dir | explicitly specify the working directory used by `ddev exec` and `ddev ssh` | `working_dir: { web:  "/var/www", db: "/etc" }` would set the working directories for the web and db containers. |
 | omit_containers | Allows the project to not load specified containers | For example, `omit_containers: [db, dba, ddev-ssh-agent]`. Currently only these containers are supported. Some containers can also be omitted globally in the ~/.ddev/global_config.yaml and the result is additive; all containers named in both places will be omitted. Note that if you omit the "db" container, several standard features of ddev that access the database container will be unusuable. |
 | nfs_mount_enabled | Allows using NFS to mount the project into the container for performance reasons | See [nfsmount_enabled documentation](../performance.md). This requires configuration on the host before it can be used. Note that project-level configuration of nfs_mount_enabled is unusual, and that if it's true in the global config, that overrides the project-specific nfs_mount_enabled|
+| fail_on_hook_fail | Decide whether `ddev start` should be interrupted by a failing hook |
 | host_https_port | Specify a specific and persistent https port for direct binding to the localhost interface | This is not commonly used, but a specific port can be provided here and the https URL will always remain the same. For example, if you put "59001", the project will always use "<https://127.0.0.1:59001".> for the localhost URL. (Note that the named URL is more commonly used and for most purposes is better.) If this is not set the port will change from `ddev start` to `ddev start` |
 | host_webserver_port | Specify a specific and persistent http port for direct binding to the localhost interface | This is not commonly used, but a specific port can be provided here and the https URL will always remain the same. For example, if you put "59000", the project will always use "<http://127.0.0.1:59000".> for the localhost URL. (Note that the named URL is more commonly used and for most purposes is better.) If this is not set the port will change from `ddev start` to `ddev start` |
 | host_db_port | localhost binding port for database server | If specified here, the database port will remain consistent. This is useful for configuration of host-side database browsers. Note, though, that `ddev sequelpro` and `ddev mysql` do all this automatically, as does the sample command `ddev mysqlworkbench`. |
@@ -54,6 +55,7 @@ The $HOME/.ddev/global_config.yaml has a few key global config options.
 | Item  | Description   | Notes  |
 |---|---|---|
 | nfs_mount_enabled | Enables NFS mounting globally for all projects | Only a "true" value has any effect. If true, NFS will be used on all projects, regardless of any settings in the individual projects. |
+| fail_on_hook_fail | Enables `ddev start` interruption globally for all projects when a hook fails | Decide whether `ddev start` should be interrupted by a failing hook |
 | omit_containers | Allows the project to not load specified containers | For example, `omit_containers: [ "dba", "ddev-ssh-agent"]`. Currently only these containers are supported. Note that you cannot omit the "db" container in the global configuration, but you can in the per-project .ddev/config.yaml. |
 | instrumentation_opt_in | Opt in or out of instrumentation reporting | If true, anonymous usage information is sent to ddev via [segment](https://segment.com) |
 | router_bind_all_interfaces | Bind on all network interfaces | If true, ddev-router will bind on all network interfaces instead of just localhost, exposing ddev projects to your local network. If you set this to true, you may consider `omit_containers: ["dba"]` so that the PHPMyAdmin port is not available.  |

pkg/ddevapp/config.go

@@ -84,6 +84,8 @@ func NewApp(appRoot string, includeOverrides bool, provider string) (*DdevApp, e
 	app.WebserverType = nodeps.WebserverDefault
 	app.NFSMountEnabled = nodeps.NFSMountEnabledDefault
 	app.NFSMountEnabledGlobal = globalconfig.DdevGlobalConfig.NFSMountEnabledGlobal
+	app.FailOnHookFail = nodeps.FailOnHookFailDefault
+	app.FailOnHookFailGlobal = globalconfig.DdevGlobalConfig.FailOnHookFailGlobal
 	app.RouterHTTPPort = nodeps.DdevDefaultRouterHTTPPort
 	app.RouterHTTPSPort = nodeps.DdevDefaultRouterHTTPSPort
 	app.PHPMyAdminPort = nodeps.DdevDefaultPHPMyAdminPort
@@ -684,6 +686,7 @@ type composeYAMLVars struct {
 	UID                       string
 	GID                       string
 	AutoRestartContainers     bool
+	FailOnHookFail            bool
 }
 
 // RenderComposeYAML renders the contents of .ddev/.ddev-docker-compose*.
@@ -740,6 +743,7 @@ func (app *DdevApp) RenderComposeYAML() (string, error) {
 		WebBuildDockerfile:        app.GetConfigPath(".webimageBuild/Dockerfile"),
 		DBBuildDockerfile:         app.GetConfigPath(".dbimageBuild/Dockerfile"),
 		AutoRestartContainers:     globalconfig.DdevGlobalConfig.AutoRestartContainers,
+		FailOnHookFail:            app.FailOnHookFail || app.FailOnHookFailGlobal,
 	}
 	if app.NFSMountEnabled || app.NFSMountEnabledGlobal {
 		templateVars.MountType = "volume"

pkg/ddevapp/ddevapp.go

@@ -80,6 +80,8 @@ type DdevApp struct {
 	MySQLVersion              string                 `yaml:"mysql_version"`
 	NFSMountEnabled           bool                   `yaml:"nfs_mount_enabled,omitempty"`
 	NFSMountEnabledGlobal     bool                   `yaml:"-"`
+	FailOnHookFail            bool                   `yaml:"fail_on_hook_fail,omitempty"`
+	FailOnHookFailGlobal      bool                   `yaml:"-"`
 	ConfigPath                string                 `yaml:"-"`
 	AppRoot                   string                 `yaml:"-"`
 	Platform                  string                 `yaml:"-"`
@@ -231,6 +233,7 @@ func (app *DdevApp) Describe(short bool) (map[string]interface{}, error) {
 	appDesc["hostname"] = app.GetHostname()
 	appDesc["hostnames"] = app.GetHostnames()
 	appDesc["nfs_mount_enabled"] = (app.NFSMountEnabled || app.NFSMountEnabledGlobal)
+	appDesc["fail_on_hook_fail"] = (app.FailOnHookFail || app.FailOnHookFailGlobal)
 	httpURLs, httpsURLs, allURLs := app.GetAllURLs()
 	appDesc["httpURLs"] = httpURLs
 	appDesc["httpsURLs"] = httpsURLs
@@ -800,12 +803,24 @@ func (app *DdevApp) ProcessHooks(hookName string) error {
 			return fmt.Errorf("unable to create task from %v", c)
 		}
 
+		if hookName == "pre-start" {
+			for k := range c {
+				if k == "exec" || k == "composer" {
+					return fmt.Errorf("pre-start hooks cannot contain %v", k)
+				}
+			}
+		}
+
 		output.UserOut.Printf("=== Running task: %s, output below", a.GetDescription())
 
 		err := a.Execute()
 
 		if err != nil {
-			output.UserOut.Errorf("task failed: %v: %v", a.GetDescription(), err)
+			if app.FailOnHookFail || app.FailOnHookFailGlobal {
+				output.UserOut.Errorf("Task failed: %v: %v", a.GetDescription(), err)
+				return fmt.Errorf("Task failed: %v", err)
+			}
+			output.UserOut.Errorf("Task failed: %v: %v", a.GetDescription(), err)
 			output.UserOut.Warn("A task failure does not mean that ddev failed, but your hook configuration has a command that failed.")
 		}
 	}

pkg/ddevapp/ddevapp_test.go

@@ -1956,12 +1956,14 @@ func TestProcessHooks(t *testing.T) {
 	testcommon.ClearDockerEnv()
 	app, err := ddevapp.NewApp(site.Dir, true, nodeps.ProviderDefault)
 	assert.NoError(err)
-	defer func() {
-		_ = app.Stop(true, false)
+	t.Cleanup(func() {
+		err = app.Stop(true, false)
+		assert.NoError(err)
 		app.Hooks = nil
-		_ = app.WriteConfig()
+		err = app.WriteConfig()
+		assert.NoError(err)
 		switchDir()
-	}()
+	})
 	err = app.Start()
 	assert.NoError(err)
 
@@ -1998,8 +2000,26 @@ func TestProcessHooks(t *testing.T) {
 	assert.FileExists(filepath.Join(app.AppRoot, fmt.Sprintf("TestProcessHooks%s.txt", app.RouterHTTPSPort)))
 	assert.FileExists(filepath.Join(app.AppRoot, "touch_works_after_and.txt"))
 
-	err = app.Stop(true, false)
+	// Attempt processing hooks with a guaranteed failure
+	app.Hooks = map[string][]ddevapp.YAMLTask{
+		"hook-test": {
+			{"exec": "ls /does-not-exist"},
+		},
+	}
+	// With default setting, ProcessHooks should succeeed
+	err = app.ProcessHooks("hook-test")
 	assert.NoError(err)
+	// With FailOnHookFail or FailOnHookFailGlobal or both, it should fail.
+	app.FailOnHookFail = true
+	err = app.ProcessHooks("hook-test")
+	assert.Error(err)
+	app.FailOnHookFail = false
+	app.FailOnHookFailGlobal = true
+	err = app.ProcessHooks("hook-test")
+	assert.Error(err)
+	app.FailOnHookFail = true
+	err = app.ProcessHooks("hook-test")
+	assert.Error(err)
 
 	runTime()
 }

pkg/ddevapp/task.go

@@ -111,17 +111,27 @@ func (c ComposerTask) GetDescription() string {
 // Returns a task (of various types) or nil
 func NewTask(app *DdevApp, ytask YAMLTask) Task {
 	if e, ok := ytask["exec-host"]; ok {
-		t := ExecHostTask{app: app, exec: e.(string)}
-		return t
+		if v, ok := e.(string); ok {
+			t := ExecHostTask{app: app, exec: v}
+			return t
+		}
+		util.Warning("Invalid exec-host value, not executing it: %v", e)
 	} else if e, ok = ytask["exec"]; ok {
-		t := ExecTask{app: app, exec: e.(string)}
-		if t.service, ok = ytask["service"].(string); !ok {
-			t.service = nodeps.WebContainer
+		if v, ok := e.(string); ok {
+			t := ExecTask{app: app, exec: v}
+			if t.service, ok = ytask["service"].(string); !ok {
+				t.service = nodeps.WebContainer
+			}
+			return t
 		}
-		return t
+		util.Warning("Invalid exec value, not executing it: %v", e)
+
 	} else if e, ok = ytask["composer"]; ok {
-		t := ComposerTask{app: app, exec: e.(string)}
-		return t
+		if v, ok := e.(string); ok {
+			t := ComposerTask{app: app, exec: v}
+			return t
+		}
+		util.Warning("Invalid composer value, not executing it: %v", e)
 	}
 	return nil
 }

pkg/ddevapp/templates.go

@@ -293,6 +293,9 @@ const ConfigInstructions = `
 # Great performance improvement but requires host configuration first.
 # See https://ddev.readthedocs.io/en/stable/users/performance/#using-nfs-to-mount-the-project-into-the-container
 
+# fail_on_hook_fail: False
+# Decide whether 'ddev start' should be interrupted by a failing hook
+
 # host_https_port: "59002"
 # The host port binding for https can be explicitly specified. It is
 # dynamic unless otherwise specified.

pkg/globalconfig/global_config.go

@@ -51,6 +51,7 @@ type GlobalConfig struct {
 	UseLetsEncrypt           bool                    `yaml:"use_letsencrypt"`
 	LetsEncryptEmail         string                  `yaml:"letsencrypt_email"`
 	AutoRestartContainers    bool                    `yaml:"auto_restart_containers"`
+	FailOnHookFailGlobal     bool                    `yaml:"fail_on_hook_fail"`
 	ProjectList              map[string]*ProjectInfo `yaml:"project_info"`
 }
 
@@ -155,6 +156,9 @@ func WriteGlobalConfig(config GlobalConfig) error {
 # ddev will ignore low values, as they're not useful
 # internet_detection_timeout_ms: 750
 
+# You can enable 'ddev start' to be interrupted by a failing hook with
+# fail_on_hook_fail: true
+
 # instrumentation_user: <your_username> # can be used to give ddev specific info about who you are
 # developer_mode: true # (defaults to false) is not used widely at this time.
 # router_bind_all_interfaces: false  # (defaults to false)
@@ -165,11 +169,11 @@ func WriteGlobalConfig(config GlobalConfig) error {
 #    exposing PHPMyAdmin.
 
 # use_hardened_images: false
-# With hardened images a container that is exposed to the internet is 
+# With hardened images a container that is exposed to the internet is
 # a harder target, although not as hard as a fully-secured host.
 # sudo is removed, mailhog is removed, and since the web container
 # is run only as the owning user, only project files might be changed
-# if a CMS or PHP bug allowed creating or altering files, and 
+# if a CMS or PHP bug allowed creating or altering files, and
 # permissions should not allow escalation.
 
 # Let's Encrypt:
@@ -181,8 +185,8 @@ func WriteGlobalConfig(config GlobalConfig) error {
 # * You will need to add a startup script to start your sites after a host reboot.
 # * If using several sites at a single top-level domain, you'll probably want to set
 #   project_tld to that top-level domain. Otherwise, you can use additional-hostnames or
-#   additional_fqdns. 
-# 
+#   additional_fqdns.
+#
 # use_letsencrypt: false
 # (Experimental, only useful on an internet-based server)
 # Set to true if certificates are to be obtained via certbot on https://letsencrypt.org/
@@ -194,6 +198,8 @@ func WriteGlobalConfig(config GlobalConfig) error {
 # Experimental
 # If true, attempt to automatically restart projects/containers after reboot or docker restart.
 
+# fail_on_hook_fail: false
+# Decide whether 'ddev start' should be interrupted by a failing hook
 
 `
 	cfgbytes = append(cfgbytes, instructions...)

pkg/nodeps/values.go

@@ -51,6 +51,9 @@ var WebserverDefault = WebserverNginxFPM
 // NFSMountEnabledDefault is default value for app.NFSMountEnabled
 var NFSMountEnabledDefault = false
 
+// FailOnHookFailDefault is the default value for app.FailOnHookFail
+var FailOnHookFailDefault = false
+
 // ValidWebserverTypes should be updated whenever supported webserver types are added or
 // removed, and should be used to ensure user-supplied values are valid.
 var ValidWebserverTypes = map[string]bool{