Skip to content

Conversation

adcoelho
Copy link
Contributor

@adcoelho adcoelho commented Sep 5, 2024

related to kibana/#186963

Summary

As a follow-up to what was done in #715 I am now adding support for another field.

Testing notes

I could not update the TestAccResourceAlertingRule tests in internal/kibana/alerting_test.go because this new field needs the actions block that requires the ID of an existing connector.

(We already supported the actions field but those were not included in the tests in the initial PR and I think the reasoning was the same. Maybe @tobio knows?).

To test I started Kibana locally and configured an MS Teams connector manually.

I then updated the test resources with the new fields and my connector ID and ran make testacc TESTARGS='-run ^TestAccResourceAlertingRule$$', which worked as expected.

Additionally, I added the file resource_rule_action_frequency.tf and tested it with Terraform CLI. (I substituted elasticstack_kibana_action_connector.index_example.connector_id for my connector ID).

I also updated the tests in internal/clients/kibana/alerting_test.go to include all possible combinations of the new fields in the Rule API responses.

@adcoelho adcoelho self-assigned this Sep 5, 2024
@adcoelho adcoelho requested review from dimuon and tobio September 5, 2024 13:05
Copy link
Member

@tobio tobio left a comment

Choose a reason for hiding this comment

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

I could not update the TestAccResourceAlertingRule tests in internal/kibana/alerting_test.go because this new field needs the actions block that requires the ID of an existing connector.

IIRC alert management was implemented before connector management. Now that we have the connector resource I think we should update the acceptance tests to create say a server_log connector so we can reference that in the actions. WDYT?

@adcoelho
Copy link
Contributor Author

adcoelho commented Sep 9, 2024

@tobio

Now that we have the connector resource I think we should update the acceptance tests to create say a server_log connector so we can reference that in the actions. WDYT?

Ah nice, I hadn't noticed the connector.go file! I can update TestAccResourceAlertingRule to include the actions but is it ok if I do that in my next PR? I have one last field to add to this resource and I can do the tests together then.

tobio
tobio previously approved these changes Sep 9, 2024
Copy link
Member

@tobio tobio left a comment

Choose a reason for hiding this comment

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

LGTM

Let's add a note about notify_when being required in 8.5 and lower and add some validation in the resource code itself enforcing that.

@tobio tobio merged commit 7802f4c into elastic:main Sep 9, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants