Skip to content
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

Add support for the alert_delay param in the Create Rule API #715

Merged
merged 13 commits into from
Aug 22, 2024

Conversation

adcoelho
Copy link
Contributor

@adcoelho adcoelho commented Aug 19, 2024

related to kibana/#186963

Summary

I am updating the Terraform Provider to use the latest version of the Create Rule API.

This is the first of a few PRs in which I intend to add support for new parameters in this API.

This PR adds support for alert_delay.

I am still testing the changes manually but updated the tests and they are all passing. I think the review can start.

What was done

I am not super familiar with this code so I'll share my steps and maybe you guys could doublecheck to see if I forgot something?

  1. Generated a new bundled.yaml following these instructions.
  2. Moved bundled.yaml to generated/alerting/bundled.yaml
  3. make generate-alerting-client
  4. Fixed errors in alerting.go etc. Not much changed in the types so I think there were no BWC issues. The only relevant thing was that some fields seem to have become optional.
  5. Re-generate mocks and fixed the tests
  6. Updated the documentation.

PS: Am I missing something in resourceRuleRead in internal/kibana/alerting.go?

@adcoelho adcoelho requested review from tobio and dimuon August 19, 2024 09:05
@adcoelho adcoelho self-assigned this Aug 19, 2024
@tobio
Copy link
Member

tobio commented Aug 19, 2024

It looks like you need to add some version checks to make sure alert_delay is only included in the request when it's supported.

@adcoelho
Copy link
Contributor Author

@tobio Thanks for your comment! I added one now to ignore the alert delay field if the version doesn't support it but I see in other places where we do version checks an error is thrown.

Is ignoring the field a suitable solution?

@tobio
Copy link
Member

tobio commented Aug 20, 2024

but I see in other places where we do version checks an error is thrown.

We should be consistent with the other resources and throw an error if alert_delay is set but not supported by the server.

internal/kibana/alerting.go Outdated Show resolved Hide resolved
@dimuon
Copy link
Contributor

dimuon commented Aug 20, 2024

internal/kibana/alerting.go Outdated Show resolved Hide resolved
internal/kibana/alerting.go Outdated Show resolved Hide resolved
@dimuon
Copy link
Contributor

dimuon commented Aug 20, 2024

@adcoelho

PS: Am I missing something in resourceRuleRead in internal/kibana/alerting.go?

We need to read alert_delay from API similar to other fields. To check this in an IT test, you may want to add a test for resource import, e.g. refer to internal/elasticsearch/cluster/slm_test.go#L47 (look for ImportState:), and check that alert_delay contains the required value.

@dimuon
Copy link
Contributor

dimuon commented Aug 20, 2024

@adcoelho

PS: Am I missing something in resourceRuleRead in internal/kibana/alerting.go?

We need to read alert_delay from API similar to other fields. To check this in an IT test, you may want to add a test for resource import, e.g. refer to internal/elasticsearch/cluster/slm_test.go#L47 (look for ImportState:), and check that alert_delay contains the required value.

Please disregard this (while import test can still be handy). alert_delay is read in ruleResponseToModel.

internal/kibana/alerting.go Outdated Show resolved Hide resolved
@adcoelho adcoelho merged commit ffa5f17 into elastic:main Aug 22, 2024
20 checks passed
tobio added a commit that referenced this pull request Aug 26, 2024
* origin/main:
  Add support for the `alert_delay` param in the Create Rule API (#715)
  chore: prepare release v0.11.6 (#716)
  Validate that mappings are a JSON object, not just valid json (#719)
  fix: move all resources in one namespace for tcp monitor acc tests (#717)
  Bump github.com/golangci/golangci-lint from 1.59.1 to 1.60.1 in /tools (#714)
  Bump github.com/docker/docker in /tools (#718)
  Bump github.com/goreleaser/goreleaser from 1.26.1 to 1.26.2 in /tools (#642)
  Bump github.com/hashicorp/terraform-plugin-framework (#705)
  Add kibana synthetics http and tcp monitor resources (#699)
  Kibana spaces data source (#682)
  Use ephemeral github token for build. (#712)
  chore: 8.15.0 is here - lets try it out (#708)
  Update changelog for 0.11.5
  Bump version for 0.11.5 (#706)
  Bugfix SLO API: Update type for `group_by` to accept either string or array-of-strings (#701)
  Support `restriction` in `elasticstack_elasticsearch_security_api_key` (#577)
  chore: follow-up CR changes for synthetics private location resource (#697)
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