Improve lakectl actions valdiate by checking required properties (#10144)

* Validate action hook properties at parse time

Add early validation of hook properties during action parsing so
configuration errors are caught at parse time instead of runtime.
Register ValidatePropertiesFunc validators for webhook (requires url),
airflow (requires url, dag_id, username, password), and lua (requires
script or script_path) hooks.

* Actions: remove hookType param from requireProperties

Move hook type context from requireProperties() into the caller's
error wrapping in Validate(), following Go's idiomatic error chaining.

Author: nopcoder

pkg/actions/action.go

@@ -96,6 +96,11 @@ func (a *Action) Validate() error {
 		if _, found := hooks[hook.Type]; !found {
 			return fmt.Errorf("hook[%d] type '%s' unknown: %w", i, hook.ID, ErrInvalidAction)
 		}
+		if validate, found := hookValidators[hook.Type]; found {
+			if err := validate(hook.Properties); err != nil {
+				return fmt.Errorf("hook[%d] type '%s': %w", i, hook.Type, err)
+			}
+		}
 	}
 	return nil
 }

pkg/actions/action_test.go

@@ -31,6 +31,13 @@ func TestAction_ReadAction(t *testing.T) {
 		{name: "invalid event type", filename: "action_invalid_event.yaml", errStr: "event 'not-a-valid-event' is not supported: invalid action"},
 		{name: "invalid yaml", filename: "action_invalid_yaml.yaml", errStr: "yaml: unmarshal errors"},
 		{name: "invalid parameter in tag event", filename: "action_invalid_param_tag_actions.yaml", errStr: "'branches' is not supported in tag event types"},
+		{name: "lua misplaced script", filename: "action_lua_missing_script.yaml", errStr: "missing hook properties"},
+		{name: "lua empty properties", filename: "action_lua_empty_properties.yaml", errStr: "'script' or 'script_path' must be supplied in properties"},
+		{name: "lua no properties", filename: "action_lua_no_properties.yaml", errStr: "missing hook properties"},
+		{name: "lua valid script", filename: "action_lua_valid_script.yaml"},
+		{name: "lua valid script_path", filename: "action_lua_valid_script_path.yaml"},
+		{name: "webhook missing url", filename: "action_webhook_missing_url.yaml", errStr: "'url' must be supplied in properties"},
+		{name: "airflow missing props", filename: "action_airflow_missing_props.yaml", errStr: "'dag_id' must be supplied in properties"},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -302,8 +309,9 @@ func TestLoadActions(t *testing.T) {
 					},
 					Hooks: []actions.ActionHook{
 						{
-							ID:   "hook_id",
-							Type: "webhook",
+							ID:         "hook_id",
+							Type:       "webhook",
+							Properties: map[string]any{"url": "https://example.com/hook"},
 						},
 					},
 				}))
@@ -326,12 +334,14 @@ func TestLoadActions(t *testing.T) {
 					},
 					Hooks: []actions.ActionHook{
 						{
-							ID:   "hook_id_1",
-							Type: "webhook",
+							ID:         "hook_id_1",
+							Type:       "webhook",
+							Properties: map[string]any{"url": "https://example.com/hook1"},
 						},
 						{
-							ID:   "hook_id_2",
-							Type: "webhook",
+							ID:         "hook_id_2",
+							Type:       "webhook",
+							Properties: map[string]any{"url": "https://example.com/hook2"},
 						},
 					},
 				}))
@@ -347,12 +357,12 @@ func TestLoadActions(t *testing.T) {
 						{
 							ID:         "hook_id_1",
 							Type:       "webhook",
-							Properties: map[string]any{},
+							Properties: map[string]any{"url": "https://example.com/hook1"},
 						},
 						{
 							ID:         "hook_id_2",
 							Type:       "webhook",
-							Properties: map[string]any{},
+							Properties: map[string]any{"url": "https://example.com/hook2"},
 						},
 					},
 				},

pkg/actions/hook.go

@@ -5,6 +5,8 @@ import (
 	"context"
 	"fmt"
 	"net/http"
+	"slices"
+	"strings"
 
 	"github.com/treeverse/lakefs/pkg/graveler"
 	"github.com/treeverse/lakefs/pkg/stats"
@@ -32,12 +34,40 @@ type HookBase struct {
 	Endpoint   *http.Server
 }
 
+type ValidatePropertiesFunc func(Properties) error
+
+// requireProperties returns a ValidatePropertiesFunc that checks required property groups.
+// Each group is a slice of keys where at least one must be present (OR within a group).
+// All groups must be satisfied (AND between groups).
+func requireProperties(groups ...[]string) ValidatePropertiesFunc {
+	return func(p Properties) error {
+		if p == nil {
+			return fmt.Errorf("missing hook properties: %w", ErrInvalidAction)
+		}
+		for _, group := range groups {
+			if !slices.ContainsFunc(group, func(key string) bool {
+				_, has := p[key]
+				return has
+			}) {
+				return fmt.Errorf("'%s' must be supplied in properties: %w", strings.Join(group, "' or '"), ErrInvalidAction)
+			}
+		}
+		return nil
+	}
+}
+
 var hooks = map[HookType]NewHookFunc{
 	HookTypeWebhook: NewWebhook,
 	HookTypeAirflow: NewAirflowHook,
 	HookTypeLua:     NewLuaHook,
 }
 
+var hookValidators = map[HookType]ValidatePropertiesFunc{
+	HookTypeWebhook: requireProperties([]string{"url"}),
+	HookTypeAirflow: requireProperties([]string{"url"}, []string{"dag_id"}, []string{"username"}, []string{"password"}),
+	HookTypeLua:     requireProperties([]string{"script", "script_path"}),
+}
+
 func NewHook(hook ActionHook, action *Action, cfg Config, server *http.Server, serverAddress string, collector stats.Collector) (Hook, error) {
 	f := hooks[hook.Type]
 	if f == nil {

pkg/actions/testdata/action_airflow_missing_props.yaml

@@ -0,0 +1,8 @@
+name: Airflow missing props
+on:
+  pre-commit:
+hooks:
+  - id: my_airflow
+    type: airflow
+    properties:
+      url: "https://airflow.example.com"

pkg/actions/testdata/action_lua_empty_properties.yaml

@@ -0,0 +1,7 @@
+name: Lua empty properties
+on:
+  pre-commit:
+hooks:
+  - id: check_metadata
+    type: lua
+    properties: {}

pkg/actions/testdata/action_lua_missing_script.yaml

@@ -0,0 +1,9 @@
+name: Lua missing script
+on:
+  pre-commit:
+hooks:
+  - id: check_metadata
+    type: lua
+    properties:
+    script: |
+        print("this is at the wrong indentation level")

pkg/actions/testdata/action_lua_no_properties.yaml

@@ -0,0 +1,6 @@
+name: Lua no properties
+on:
+  pre-commit:
+hooks:
+  - id: check_metadata
+    type: lua

pkg/actions/testdata/action_lua_valid_script.yaml

@@ -0,0 +1,9 @@
+name: Lua valid script
+on:
+  pre-commit:
+hooks:
+  - id: check_metadata
+    type: lua
+    properties:
+      script: |
+        print("hello")

pkg/actions/testdata/action_lua_valid_script_path.yaml

@@ -0,0 +1,8 @@
+name: Lua valid script path
+on:
+  pre-commit:
+hooks:
+  - id: check_metadata
+    type: lua
+    properties:
+      script_path: scripts/check.lua

pkg/actions/testdata/action_webhook_missing_url.yaml

@@ -0,0 +1,8 @@
+name: Webhook missing url
+on:
+  pre-commit:
+hooks:
+  - id: my_webhook
+    type: webhook
+    properties:
+      timeout: 30s