diff --git a/internal/terraform/context_validate_test.go b/internal/terraform/context_validate_test.go index 67beadb889fc..9344bbc0bfd8 100644 --- a/internal/terraform/context_validate_test.go +++ b/internal/terraform/context_validate_test.go @@ -3575,60 +3575,114 @@ func TestContext2Validate_queryList(t *testing.T) { // Action Validation is largely exercised in context_plan_actions_test.go func TestContext2Validate_action(t *testing.T) { tests := map[string]struct { - config string - wantErr string + config string + wantErr string + expectValidateCalled bool }{ "valid config": { ` -terraform { - required_providers { - test = { - source = "hashicorp/test" - version = "1.0.0" + terraform { + required_providers { + test = { + source = "hashicorp/test" + version = "1.0.0" + } + } + } + action "test_register" "foo" { + config { + host = "cmdb.snot" + } + } + resource "test_instance" "foo" { + lifecycle { + action_trigger { + events = [after_create] + actions = [action.test_register.foo] + } + } + } + `, + "", + true, + }, + "validly null config": { // this doesn't seem likely, but let's make sure nothing panics. + ` + terraform { + required_providers { + test = { + source = "hashicorp/test" + version = "1.0.0" + } + } } - } -} -action "test_register" "foo" { - config { - host = "cmdb.snot" - } -} -resource "test_instance" "foo" { - lifecycle { - action_trigger { - events = [after_create] - actions = [action.test_register.foo] - } - } -} -`, + action "test_other" "foo" { + } + resource "test_instance" "foo" { + lifecycle { + action_trigger { + events = [after_create] + actions = [action.test_other.foo] + } + } + } + `, "", + true, }, "missing required config": { ` -terraform { - required_providers { - test = { - source = "hashicorp/test" - version = "1.0.0" + terraform { + required_providers { + test = { + source = "hashicorp/test" + version = "1.0.0" + } + } } - } -} -action "test_register" "foo" { - config {} -} -resource "test_instance" "foo" { - lifecycle { - action_trigger { - events = [after_create] - actions = [action.test_register.foo] - } - } -} -`, - "host is null", + action "test_register" "foo" { + config {} + } + resource "test_instance" "foo" { + lifecycle { + action_trigger { + events = [after_create] + actions = [action.test_register.foo] + } + } + } + `, + "Missing required argument: The argument \"host\" is required, but no definition was found.", + false, + }, + "invalid required config (provider validation)": { + ` + terraform { + required_providers { + test = { + source = "hashicorp/test" + version = "1.0.0" + } + } + } + action "test_register" "foo" { + config { + host = "invalid" + } + } + resource "test_instance" "foo" { + lifecycle { + action_trigger { + events = [after_create] + actions = [action.test_register.foo] + } + } + } + `, + "Invalid value for required argument \"host\" because I said so", + true, }, - "invalid nil config config": { + "invalid nil config": { ` terraform { required_providers { @@ -3649,7 +3703,8 @@ resource "test_instance" "foo" { } } `, - "config is null", + "Missing required argument: The argument \"host\" is required, but was not set.", + false, }, } @@ -3669,6 +3724,14 @@ resource "test_instance" "foo" { }, Actions: map[string]*providers.ActionSchema{ "test_register": { + ConfigSchema: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "host": {Type: cty.String, Required: true}, + "output": {Type: cty.String, Computed: true, Optional: true}, + }, + }, + }, + "test_other": { ConfigSchema: &configschema.Block{ Attributes: map[string]*configschema.Attribute{ "host": {Type: cty.String, Optional: true}, @@ -3678,12 +3741,8 @@ resource "test_instance" "foo" { }, }) p.ValidateActionConfigFn = func(r providers.ValidateActionConfigRequest) (resp providers.ValidateActionConfigResponse) { - if r.Config.IsNull() { - resp.Diagnostics = resp.Diagnostics.Append(errors.New("config is null")) - return - } - if r.Config.GetAttr("host").IsNull() { - resp.Diagnostics = resp.Diagnostics.Append(errors.New("host is null")) + if r.Config.GetAttr("host") == cty.StringVal("invalid") { + resp.Diagnostics = resp.Diagnostics.Append(errors.New("Invalid value for required argument \"host\" because I said so")) } return } @@ -3695,10 +3754,6 @@ resource "test_instance" "foo" { }) diags := ctx.Validate(m, nil) - if !p.ValidateActionConfigCalled { - t.Fatal("ValidateAction RPC was not called") - } - if test.wantErr != "" { if !diags.HasErrors() { t.Errorf("unexpected success\nwant errors: %s", test.wantErr) @@ -3710,6 +3765,13 @@ resource "test_instance" "foo" { t.Errorf("unexpected error\ngot: %s", diags.Err().Error()) } } + if p.ValidateActionConfigCalled != test.expectValidateCalled { + if test.expectValidateCalled { + t.Fatal("provider Validate RPC was expected, but not called") + } else { + t.Fatal("unexpected Validate RCP call") + } + } }) } } diff --git a/internal/terraform/node_action_abstract.go b/internal/terraform/node_action_abstract.go index 2af00048d56c..3ca9a55b0d69 100644 --- a/internal/terraform/node_action_abstract.go +++ b/internal/terraform/node_action_abstract.go @@ -44,9 +44,8 @@ func (n NodeAbstractAction) Name() string { // abstract action to a concrete one of some type. type ConcreteActionNodeFunc func(*NodeAbstractAction) dag.Vertex -// I'm not sure why my ConcreteActionNodeFUnction kept being nil in tests, but -// this is much more robust. If it isn't a validate walk, we need -// nodeExpandActionDeclaration. +// DefaultConcreteActionNodeFunc is the default ConcreteActionNodeFunc used by +// everything except validate. func DefaultConcreteActionNodeFunc(a *NodeAbstractAction) dag.Vertex { return &nodeExpandActionDeclaration{ NodeAbstractAction: a, diff --git a/internal/terraform/node_action_validate.go b/internal/terraform/node_action_validate.go index 4ad63a7254d1..8c36de6309ec 100644 --- a/internal/terraform/node_action_validate.go +++ b/internal/terraform/node_action_validate.go @@ -82,16 +82,24 @@ func (n *NodeValidatableAction) Execute(ctx EvalContext, _ walkOperation) tfdiag return diags } - var configVal cty.Value - var valDiags tfdiags.Diagnostics - if n.Config.Config != nil { - configVal, _, valDiags = ctx.EvaluateBlock(n.Config.Config, schema.ConfigSchema, nil, keyData) - diags = diags.Append(valDiags) - if valDiags.HasErrors() { + config := n.Config.Config + if n.Config.Config == nil { + config = hcl.EmptyBody() + } + + configVal, _, valDiags := ctx.EvaluateBlock(config, schema.ConfigSchema, nil, keyData) + if valDiags.HasErrors() { + // If there was no config block at all, we'll add a Context range to the returned diagnostic + if n.Config.Config == nil { + for _, diag := range valDiags.ToHCL() { + diag.Context = &n.Config.DeclRange + diags = diags.Append(diag) + } + return diags + } else { + diags = diags.Append(valDiags) return diags } - } else { - configVal = cty.NullVal(n.Schema.ConfigSchema.ImpliedType()) } // Use unmarked value for validate request