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

feat(alert_muting_rule): Add action_on_muting_rule_window_ended attribute in newrelic_alert_muting_rule Terraform Resource #2783

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

Aashirwadjain
Copy link
Collaborator

@Aashirwadjain Aashirwadjain commented Dec 17, 2024

Description

  • The addition of the action_on_muting_rule_window_ended attribute in the newrelic_alert_muting_rule Terraform resource aims to enhance the functionality of muting rules within New Relic's alerting system using terraform.
  • The attribute action_on_muting_rule_window_ended is added to MutingRule Schema Resource and handler methods.
  • Link to GitHub Issue: Resource newrelic_alert_muting_rule: Support for Muting Rule "End Behaviour" #2774

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist:

Please delete options that are not relevant.

  • My commit message follows conventional commits
  • My code is formatted to Go standards
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes. Go here for instructions on running tests locally.

@Aashirwadjain Aashirwadjain changed the title Muting rule end behaviour feat(alerts-muting-rule): Add end_behaviour attribute in newrelic_alert_muting_rule Terraform Resource Dec 17, 2024
Copy link
Contributor

@vagrawal-newrelic vagrawal-newrelic left a comment

Choose a reason for hiding this comment

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

approved

@pranav-new-relic pranav-new-relic changed the title feat(alerts-muting-rule): Add end_behaviour attribute in newrelic_alert_muting_rule Terraform Resource feat(alert_muting_rule): Add end_behaviour attribute in newrelic_alert_muting_rule Terraform Resource Dec 18, 2024
@pranav-new-relic
Copy link
Member

pranav-new-relic commented Dec 18, 2024

@Aashirwadjain please add documentation changes (I've pointed where the documentation is to be added below), without which customers would not be aware of the addition of this new attribute.

## Argument Reference
The following arguments are supported:
* `account_id` - (Optional) The account id of the MutingRule.
* `condition` - (Required) The condition that defines which incidents to target. See [Nested condition blocks](#nested-condition-blocks) below for details.
* `enabled` - (Required) Whether the MutingRule is enabled.
* `name` - The name of the MutingRule.
* `description` - The description of the MutingRule.
* `schedule` - (Optional) Specify a schedule for enabling the MutingRule. See [Schedule](#schedule) below for details

@pranav-new-relic
Copy link
Member

@Aashirwadjain in addition, please add relevant changes to integration tests, to make sure this new attribute is also part of the configuration that is created, read, and updated by the integration test for muting rules.

Please see the following file, add wherever appropriate. By this, I mean - you don't have to add the attribute to all tests, just make sure the attribute is added wherever needed to make sure atleast one test fully covers this attribute too, along with other attributes of the resource (during create, read, update).

@pranav-new-relic
Copy link
Member

pranav-new-relic commented Dec 18, 2024

@Aashirwadjain one occurrence that you will also find while testing your changes :)

Screen.Recording.2024-12-19.at.12.18.13.AM.mov
  • compile your local changes and try using a configuration of the resource newrelic_alert_muting_rule, but without the new attribute you've added. Perform a terraform plan and terraform apply to create a brand new muting rule.
  • immediately after the above, run a terraform plan. Since no external changes were made in the UI, or in the Terraform configuration, the expectation of this terraform plan would be a "no changes" output, but you see a that a drift is being shown, of a change that the customer did not make, with regard to this new attribute.

Think of why this is happening, and what needs to be added/deleted/fixed in the code to prevent this occurrence. Fixing this is important, otherwise this would affect current customers using this Terraform resource without the new attribute (if these changes are merged without the fix, they would see a change upon terraform plan they did not make, or have no idea about).

Please rigorously test your changes to help fill such gaps :)

…muting_rule_window_ended and validation method
@Aashirwadjain
Copy link
Collaborator Author

Thanks for suggestions @pranav-new-relic, Following changes are made accordingly:

  • Documentation is updated and added description of action_on_muting_rule_window_ended attribute.
  • action_on_muting_rule_window_ended attribute is added to integration tests.
  • flattenMutingRule method is updated and added validations.

@pranav-new-relic
Copy link
Member

image

@Aashirwadjain Aashirwadjain changed the title feat(alert_muting_rule): Add end_behaviour attribute in newrelic_alert_muting_rule Terraform Resource feat(alert_muting_rule): Add action_on_muting_rule_window_ended attribute in newrelic_alert_muting_rule Terraform Resource Dec 31, 2024
if _, ok := d.GetOk("action_on_muting_rule_window_ended"); ok {
_ = d.Set("action_on_muting_rule_window_ended", mutingRule.ActionOnMutingRuleWindowEnded)
}
_ = d.Set("action_on_muting_rule_window_ended", mutingRule.ActionOnMutingRuleWindowEnded)
Copy link
Member

Choose a reason for hiding this comment

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

@vagrawal-newrelic @Aashirwadjain this is nice, but the attribute doesn't need to be Computed for this to happen since the read function is anyway being called in create; you can just have this condition.

The only thing to double check is that since this is coming from an enum in client go, if there is no value returned from the API for this attribute, is an empty string effectively being sent to Terraform from the Go Client via this attribute and saved as "" to the state; if this is the case, we should be okay; and if not, a condition might need to be added accordingly. I don't think this should be the case though.

Please test these changes (for all cases we ideated previously) and let me know if you discover something unusual

@@ -54,7 +55,7 @@ The following arguments are supported:
* `name` - The name of the MutingRule.
* `description` - The description of the MutingRule.
* `schedule` - (Optional) Specify a schedule for enabling the MutingRule. See [Schedule](#schedule) below for details

* `action_on_muting_rule_window_ended` - (Optional) The action when the muting rule window is ended or disabled. Valid values are `CLOSE_ISSUES_ON_INACTIVE`, `DO_NOTHING`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* `action_on_muting_rule_window_ended` - (Optional) The action when the muting rule window is ended or disabled. Valid values are `CLOSE_ISSUES_ON_INACTIVE`, `DO_NOTHING`
* `action_on_muting_rule_window_ended` - (Optional) The action when the muting rule window is ended or disabled. Valid values are `CLOSE_ISSUES_ON_INACTIVE`, `DO_NOTHING`.

just a . :)

@pranav-new-relic
Copy link
Member

pranav-new-relic commented Jan 1, 2025

we should be good to merge once the requested changes are done (and further reviewed by VIshal/me), but ⚠️

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