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

Jump to bottom
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

merged 2 commits into from
Sep 19, 2025

Conversation

@mildwonkey
Copy link
Contributor

@mildwonkey mildwonkey commented Sep 16, 2025 •
edited
Loading

This PR fixes terraform's handling of nil action configurations. It will now properly return errors about missing required (not computed) attributes. This is done by substituting an hcl.EmptyBody in place of a nil config.

This would have gone a lot faster if I hadn't been slightly wrong about the hcl.EmptyBody from the start; I'd thought it was one of my error cases 😂😭

Fixes #

Target Release

1.14.x

Rollback Plan

  • If a change needs to be reverted, we will roll out an update to the code within 7 days.

Changes to Security Controls

Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.

CHANGELOG entry

  • This change is user-facing and I added a changelog entry.
  • This change is not user-facing.

@mildwonkey mildwonkey requested a review from a team as a code owner September 16, 2025 18:29
@mildwonkey mildwonkey added no-changelog-needed Add this to your PR if the change does not require a changelog entry 1.14-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged labels Sep 16, 2025
liamcervante
liamcervante reviewed Sep 16, 2025
View reviewed changes
dsa0x
dsa0x reviewed Sep 16, 2025
View reviewed changes
@mildwonkey
actions: return an error if config is null but the schema has require…
2084154 
…d attributes

There are various methods in terraform that will let you know if you are missing required attributes - none of which work with hcl.EmptyBody or nil inputs. This adds some helper methods so terraform can check if an action schema has required arguments and returns an error if the config is null but arguments are required.

I did not bother with an exhaustive list of missing args, but I'm open to making that change. I'd like to get eyes on this section before I move on to modifying what we send to the provider.
@mildwonkey mildwonkey force-pushed the mildwonkey/action-config branch from 24abd1e to 2084154 Compare September 17, 2025 14:59
@mildwonkey mildwonkey changed the title actions: return an error if config is null but the schema has required attrs actions: return an error if config is omitted but the schema has required attrs Sep 17, 2025
@mildwonkey mildwonkey requested review from a team and liamcervante September 17, 2025 15:05
dsa0x
dsa0x reviewed Sep 18, 2025
View reviewed changes
internal/terraform/context_validate_test.go
Comment on lines +3706 to +3707
"Missing required argument: The argument \"host\" is required, but was not set.",
false,
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

jbardin
jbardin reviewed Sep 18, 2025
View reviewed changes
@mildwonkey mildwonkey force-pushed the mildwonkey/action-config branch from be49fbd to 8a889aa Compare September 18, 2025 17:13
@mildwonkey mildwonkey requested a review from jbardin September 18, 2025 19:22
@mildwonkey mildwonkey merged commit e241e1a into main Sep 19, 2025
7 checks passed
@mildwonkey mildwonkey deleted the mildwonkey/action-config branch September 19, 2025 12:54
@github-actions github-actions bot mentioned this pull request Sep 19, 2025
Merged
3 tasks
@github-actions
Copy link
Contributor

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 20, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@dsa0x dsa0x dsa0x left review comments

@jbardin jbardin jbardin approved these changes

@liamcervante liamcervante Awaiting requested review from liamcervante

Assignees

No one assigned

Labels

1.14-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged no-changelog-needed Add this to your PR if the change does not require a changelog entry

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mildwonkey @jbardin @dsa0x @liamcervante