-
Notifications
You must be signed in to change notification settings - Fork 17
Add ActionChecks
#570
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.
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
base: main
Are you sure you want to change the base?
Add ActionChecks
#570
Conversation
SBGoods
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution @YakDriver, I like the overall concept of using interceptors to capture progress messages. I have a couple of comments regarding overall design and some of the functionality.
| func TestCheckProgressMessageContains(actionName, expectedContent string) actioncheck.ActionCheck { | ||
| return actioncheck.ExpectProgressMessageContains(actionName, expectedContent) | ||
| } | ||
|
|
||
| // TestCheckProgressMessageCount returns an ActionCheck that verifies the expected number of progress messages. | ||
| // | ||
| // Example usage: | ||
| // | ||
| // resource.TestStep{ | ||
| // Config: testConfig, | ||
| // ActionChecks: []actioncheck.ActionCheck{ | ||
| // resource.TestCheckProgressMessageCount("aws_lambda_invoke.test", 2), | ||
| // }, | ||
| // } | ||
| func TestCheckProgressMessageCount(actionName string, expectedCount int) actioncheck.ActionCheck { | ||
| return actioncheck.ExpectProgressCount(actionName, expectedCount) | ||
| } | ||
|
|
||
| // TestCheckProgressMessageSequence returns an ActionCheck that verifies progress messages appear in the expected sequence. | ||
| // | ||
| // Example usage: | ||
| // | ||
| // resource.TestStep{ | ||
| // Config: testConfig, | ||
| // ActionChecks: []actioncheck.ActionCheck{ | ||
| // resource.TestCheckProgressMessageSequence("aws_lambda_invoke.test", []string{"Invoking", "completed successfully"}), | ||
| // }, | ||
| // } | ||
| func TestCheckProgressMessageSequence(actionName string, expectedSequence []string) actioncheck.ActionCheck { | ||
| return actioncheck.ExpectProgressSequence(actionName, expectedSequence) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For newer checks, we would prefer the functions to be in the same file where it's struct is defined for readability. This also helps distinguish the checks by package name:
actioncheck.TestCheckProgressMessageCount("framework_modify_file_action", 2),| // resource.TestCheckProgressMessageContains("aws_lambda_invoke.test", "Lambda function logs:"), | ||
| // }, | ||
| // } | ||
| func TestCheckProgressMessageContains(actionName, expectedContent string) actioncheck.ActionCheck { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| func TestCheckProgressMessageContains(actionName, expectedContent string) actioncheck.ActionCheck { | |
| func ExpectProgressMessageContains(actionName, expectedContent string) actioncheck.ActionCheck { |
| // resource.TestCheckProgressMessageCount("aws_lambda_invoke.test", 2), | ||
| // }, | ||
| // } | ||
| func TestCheckProgressMessageCount(actionName string, expectedCount int) actioncheck.ActionCheck { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| func TestCheckProgressMessageCount(actionName string, expectedCount int) actioncheck.ActionCheck { | |
| func ExpectProgressMessageCount(actionName string, expectedCount int) actioncheck.ActionCheck { |
| // resource.TestCheckProgressMessageSequence("aws_lambda_invoke.test", []string{"Invoking", "completed successfully"}), | ||
| // }, | ||
| // } | ||
| func TestCheckProgressMessageSequence(actionName string, expectedSequence []string) actioncheck.ActionCheck { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| func TestCheckProgressMessageSequence(actionName string, expectedSequence []string) actioncheck.ActionCheck { | |
| func ExpectProgressMessageSequence(actionName string, expectedSequence []string) actioncheck.ActionCheck { |
| `, | ||
| ActionChecks: []actioncheck.ActionCheck{ | ||
| // Check that the action produces a message containing log output | ||
| resource.TestCheckProgressMessageContains("aws_lambda_invoke.test", "Lambda function logs:"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| resource.TestCheckProgressMessageContains("aws_lambda_invoke.test", "Lambda function logs:"), | |
| resource.TestCheckProgressMessageContains("aws_lambda_invoke", "Lambda function logs:"), |
The examples as written do not work because the provider does not have access to the alias name so you can only reference messages by action type name.
| resource.TestCheckProgressMessageCount("aws_lambda_invoke.test", 2), | ||
|
|
||
| // Check that messages appear in the expected sequence | ||
| resource.TestCheckProgressMessageSequence("aws_lambda_invoke.test", []string{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| resource.TestCheckProgressMessageSequence("aws_lambda_invoke.test", []string{ | |
| resource.TestCheckProgressMessageSequence("aws_lambda_invoke", []string{ |
| func runActionChecks(ctx context.Context, actionChecks []actioncheck.ActionCheck, wd *plugintest.WorkingDir) error { | ||
| progressCapture := wd.GetProgressCapture() | ||
|
|
||
| // IMPORTANT: Unlike StateChecks and PlanChecks which retrieve persisted data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really like this comment clarifying the source of the progress messages!
| if req.ActionName != e.actionName { | ||
| return | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now, if the provider developer mistypes the action name in the check or if the expected action never produces any progress messages, the check will still pass without an error.
I think that we need to pass the entire map of messages from ProcessCapture into the CheckActionRequest so that each action check can throw an error if the action type name doesn't exist in the map.
| ) | ||
|
|
||
| // ActionCheck defines an interface for implementing test logic that checks action progress messages. | ||
| type ActionCheck interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I wonder if the interface name should make it clear that these checks assert against intercepted provider messages rather than a standard Terraform artifact. Maybe there could be a future where there is a terraform-produced artifact for Actions and we would like a different interface for those types of checks? I don't have a strong opinion on this right now 🤔
| t.Helper() | ||
|
|
||
| // Enable progress capture if action checks are defined - do this early before any provider operations | ||
| var progressCaptureEnabled bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like having this flag for process capture here, it helps reduce the performance cost of the interceptors.
Description
NOTE: The Use modern Go commit adds about 400 lines of modern Go cleanup. I can split that from these changes if you prefer.
This PR adds
ActionChecks- a new testing capability that validates progress messages from Terraform actions during test execution.New
ActionChecksframework following the same patterns as existingStateChecksandPlanChecks:TestCheckProgressMessageContains()- verify messages contain expected contentTestCheckProgressMessageCount()- verify expected number of messagesTestCheckProgressMessageSequence()- verify messages appear in expected orderProvider protocol interception to capture action progress messages in real-time:
InvokeActioncalls to captureresp.SendProgress()messagesIntegration with
TestStep:ActionChecks[]actioncheck.ActionCheckfield in TestStepActionChecksare presentUsage example:
This enables testing of action implementations like AWS Lambda invoke, CloudFront invalidation, etc. to verify they produce expected progress output for users.
Rollback Plan
Changes to Security Controls
Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.