Skip to content

actions: return an error if config is omitted but the schema has required attrs #37626

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Sep 19, 2025
Merged

actions: return an error if config is omitted but the schema has required attrs #37626

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2084154
actions: return an error if config is null but the schema has require…
mildwonkey Sep 16, 2025
8a889aa
generate nicer errors
mildwonkey Sep 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
172 changes: 117 additions & 55 deletions internal/terraform/context_validate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -3649,7 +3703,8 @@ resource "test_instance" "foo" {
}
}
`,
"config is null",
"Missing required argument: The argument \"host\" is required, but was not set.",
false,
Comment on lines +3706 to +3707
Copy link
Member

@dsa0x dsa0x Sep 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a more succinct solution, and I was pondering if to borrow the same idea. However, is it clear enough given that this error simply indicates that the argument "host", with no mention to "config" at all?

There is also no source diagnostic, so the user has to figure out that the host belongs within a config block.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got it as close as I'm going to right now - I absolutely agree with you, and this still doesn't have the nice error I'd like it to, but I added some context and hopefully we can call this good enough for now and I will add it to my list of things to improve

},
}

Expand All @@ -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},
Expand All @@ -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
}
Expand All @@ -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)
Expand All @@ -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")
}
}
})
}
}
5 changes: 2 additions & 3 deletions internal/terraform/node_action_abstract.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
20 changes: 11 additions & 9 deletions internal/terraform/node_action_validate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,16 +82,18 @@ 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() {
return diags
}
// If the action has no "config" attribute at all, we'll send the provider
// an empty body - config = {} and omitted config entirely are the same, internally.
var config hcl.Body
if n.Config.Config == nil {
config = hcl.EmptyBody()
} else {
configVal = cty.NullVal(n.Schema.ConfigSchema.ImpliedType())
config = n.Config.Config
}
configVal, _, valDiags := ctx.EvaluateBlock(config, schema.ConfigSchema, nil, keyData)
diags = diags.Append(valDiags)
if valDiags.HasErrors() {
return diags
}

// Use unmarked value for validate request
Expand Down