Issue 463: Introduce catch_all #481

speedfl · 2022-04-13T21:30:08Z

Reference: #463

Proposal is to introduce catch_all parameter as parameter to reflect what is in the API.

Then if catch_all is present, as catch-all rule is created at Ruleset creation, an update is performed instead of a create (PUT instead of POST)

Concerning the delete, I am skipping it but maybe it could be better to do another update to reset the catch-all rule (to be discussed in this thread)

Concerning the the conditions as mentioned, it is optional. So in case of catch_all it must be removed

speedfl · 2022-04-14T18:53:21Z

I took a look to the deletion, I confirm that catch_all rule cannot be deleted and it results in an error:

DELETE API call to https://api.pagerduty.com/rulesets/ID/rules/ID failed 400 Bad Request. Code: 0, Errors: map[rules:[should have at least 1 rule]], Message: Invalid payload

I tried to apply this code:

// Don't delete catch_all resource
if _, ok := d.GetOk(("catch_all")); ok {

	log.Printf("[INFO] Rule %s is a catch_all rule, don't delete it, reset it instead", d.Id())

	rule, _, err := client.Rulesets.GetRule(rulesetID, d.Id())

	if err != nil {
		return err
	}

	rule.Actions = nil
	rule.TimeFrame = nil

	if err := performRulesetRuleUpdate(rulesetID, d.Id(), rule, client); err != nil {
		return err
	}

	d.SetId("")

	return nil
}

But the PUT action is not removing the route.value (even if it is not in the input)

Input payload

{
 "rule": {
  "id": "ID",
  "position": 0,
  "disabled": false,
  "self": "SELF",
  "catch_all": true
 }
}

I also tried this:

rule.Actions = new(pagerduty.RuleActions)

Input payload

{
 "rule": {
  "id": "ID",
  "position": 0,
  "actions": {},
  "disabled": false,
  "self": "SELF",
  "catch_all": true
 }
}

and

rule.Actions = new(pagerduty.RuleActions)
rule.Actions.Route = new(pagerduty.RuleActionParameter)

Input payload

{
 "rule": {
  "id": "ID",
  "position": 0,
  "actions": {
      "route": {}
   },
  "disabled": false,
  "self": "SELF",
  "catch_all": true
 }
}

The route to service is not removed.

This looks like a PATCH feature

If I remember well in API design:

the PATCH is only updating the elements with a value and does not remove elements not provided (null)
the PUT is a full replace (or create if resource does not exists).

Here as I am using the PUT the resource must be fully replaced with actions = null (or empty)

Waiting for your answer

speedfl · 2022-04-20T12:03:42Z

Any news ?

Signed-off-by: Gavin Reynolds <[email protected]>

…ault Signed-off-by: Gavin Reynolds <[email protected]>

Signed-off-by: Gavin Reynolds <[email protected]>

gsreynolds · 2022-04-22T19:55:48Z

Hi @speedfl - thanks for your first-time contribution! Nicely done!

I've taken a look through your pull request and added some appropriate tests, as well as a little documentation.

I've also taken a look at your issue with deletion/resetting a catch_all rule. Using Get an Event Rule and Update an Event Rule requests from the API Reference, the default state we would want to reset to when "deleting" the catch_all rule would be more like:

{
    "rule_id": "ID",
    "rule": {
        "actions": {
            "annotate": null,
            "event_action": null,
            "extractions": [],
            "priority": null,
            "route": null,
            "severity": null,
            "suppress": {
                "value": true
            },
            "suspend": null
        },
        "catch_all": true,
        "disabled": false,
        "position": 0
    }
}

So we need to do something more like this in the resource:

// Reset all available actions back to the default state of the catch_all rule
rule.Actions.Annotate = nil
rule.Actions.EventAction = nil
rule.Actions.Extractions = nil
rule.Actions.Priority = nil
rule.Actions.Route = nil
rule.Actions.Severity = nil
rule.Actions.Suppress.Value = true
rule.Actions.Suspend = nil

However, what also needed to be changed was to stop omitting those keys if the values were nil, which I've also done in the type RuleActions struct.

Debug log excerpts from the catch_all rule "deletion"/reset, successfully resetting the catch_all rule back to the default state debug_log_catch_all_reset.log].

(If we are about to delete the ruleset, this probably doesn't matter much. If we were to stop managing the catch_all rule by removing the matching resource in your Terraform code, it probably does matter.)

and finally the test output:

go clean -testcache && TF_ACC=1 go test -run "TestAccPagerDutyRuleset" ./pagerduty -v -timeout 120m
=== RUN   TestAccPagerDutyRulesetRule_import
--- PASS: TestAccPagerDutyRulesetRule_import (32.67s)
=== RUN   TestAccPagerDutyRuleset_import
--- PASS: TestAccPagerDutyRuleset_import (32.76s)
=== RUN   TestAccPagerDutyRulesetWithNoTeam_import
--- PASS: TestAccPagerDutyRulesetWithNoTeam_import (27.60s)
=== RUN   TestAccPagerDutyRulesetRule_Basic
--- PASS: TestAccPagerDutyRulesetRule_Basic (44.10s)
=== RUN   TestAccPagerDutyRulesetRule_MultipleRules
--- PASS: TestAccPagerDutyRulesetRule_MultipleRules (29.24s)
=== RUN   TestAccPagerDutyRulesetRule_CatchAllRule
--- PASS: TestAccPagerDutyRulesetRule_CatchAllRule (26.42s)
=== RUN   TestAccPagerDutyRulesetRule_CatchAllRuleRoute
--- PASS: TestAccPagerDutyRulesetRule_CatchAllRuleRoute (23.88s)
=== RUN   TestAccPagerDutyRuleset_Basic
--- PASS: TestAccPagerDutyRuleset_Basic (55.07s)
PASS
ok      github.com/terraform-providers/terraform-provider-pagerduty/pagerduty   272.156s

speedfl · 2022-04-22T20:16:13Z

Thanks a lot for your help @gsreynolds !

gsreynolds · 2022-04-22T20:45:23Z

@stmcallister we'll need to merge heimweh/go-pagerduty#84 and revendor here, FYI.

speedfl · 2022-04-27T12:43:08Z

@gsreynolds hope you are doing well heimweh/go-pagerduty#84 has been merged. I will make a try

…uty into bugfix/463

speedfl · 2022-04-27T19:02:35Z

@gsreynolds I did a fix of an NPE.

After validation:
terraform apply ok / terraform destroy ok.

According to the destroy logs we can see that routing is removed

2022-04-27T14:51:14.393-0400 [INFO]  provider.terraform-provider-pagerduty_v2.4.0: 2022/04/27 14:51:14 [INFO] Rule c7ee4606-f3cb-437d-af51-48ad7952493c is a catch_all rule, don't delete it, reset it instead: timestamp=2022-04-27T14:51:14.393-0400
...
GET /rulesets/e4809916-6fd8-4e3c-aad1-9ac242ce8d1c/rules/c7ee4606-f3cb-437d-af51-48ad7952493c
... Response
{
 "rule": {
  "actions": {
   "annotate": null,
   "automation_actions": [],
   "event_action": null,
   "extractions": [],
   "priority": null,
   "route": {
    "value": "PQ3OTTP"
   },
   "severity": null,
   "suppress": null,
   "suspend": null
  },
  "catch_all": true,
  "disabled": false,
  "id": "c7ee4606-f3cb-437d-af51-48ad7952493c",
  "position": 0,
  "self": "https://api.pagerduty.com/rulesets/e4809916-6fd8-4e3c-aad1-9ac242ce8d1c/rules/c7ee4606-f3cb-437d-af51-48ad7952493c"
 }
}
...
PUT /rulesets/e4809916-6fd8-4e3c-aad1-9ac242ce8d1c/rules/c7ee4606-f3cb-437d-af51-48ad7952493c
{
 "rule": {
  "id": "c7ee4606-f3cb-437d-af51-48ad7952493c",
  "position": 0,
  "disabled": false,
  "actions": {
   "suppress": {
    "value": true
   },
   "annotate": null,
   "severity": null,
   "priority": null,
   "route": null,
   "event_action": null,
   "suspend": null
  },
  "self": "https://api.pagerduty.com/rulesets/e4809916-6fd8-4e3c-aad1-9ac242ce8d1c/rules/c7ee4606-f3cb-437d-af51-48ad7952493c",
  "catch_all": true
 }
}
... Response
{
 "rule": {
  "actions": {
   "annotate": null,
   "automation_actions": [],
   "event_action": null,
   "extractions": [],
   "priority": null,
   "route": {
    "value": "PQ3OTTP"
   },
   "severity": null,
   "suppress": null,
   "suspend": null
  },
  "catch_all": true,
  "disabled": false,
  "id": "c7ee4606-f3cb-437d-af51-48ad7952493c",
  "position": 0,
  "self": "https://api.pagerduty.com/rulesets/e4809916-6fd8-4e3c-aad1-9ac242ce8d1c/rules/c7ee4606-f3cb-437d-af51-48ad7952493c"
 }
}

If you want to validate on your side you can use:

terraform {
  required_version = ">= 1.0"

  required_providers {
    pagerduty = {
      source  = "hashicorp/pagerduty"
      version = "2.4.0"
    }
  }
}

provider "pagerduty" {
}

data "pagerduty_user" "foo" {
  email = "[email protected]"
}

resource "pagerduty_escalation_policy" "foo" {
  name      = "Engineering Escalation Policy"
  num_loops = 2

  rule {
    escalation_delay_in_minutes = 10

    target {
      type = "user_reference"
      id   = data.pagerduty_user.foo.id
    }
  }
}

resource "pagerduty_service" "foo" {
  name                    = "My Web App"
  auto_resolve_timeout    = 14400
  acknowledgement_timeout = 600
  escalation_policy       = pagerduty_escalation_policy.foo.id
  alert_creation          = "create_alerts_and_incidents"
}

resource "pagerduty_team" "foo" {
  name = "Engineering (Seattle)"
}

resource "pagerduty_ruleset" "foo" {
  name = "Primary Ruleset"
  team {
    id = pagerduty_team.foo.id
  }
}

resource "pagerduty_ruleset_rule" "foo" {
  ruleset  = pagerduty_ruleset.foo.id
  position = 0
  disabled = "false"
  catch_all = "true"
  actions {
    route {
      value = pagerduty_service.foo.id
    }
  }
}

For info I am using https://www.terraform.io/cli/config/config-file#filesystem_mirror (I put in hashicorp/pagerduty)

speedfl · 2022-04-29T15:34:43Z

@gsreynolds any news ? :)

stmcallister

Very cool! Thank you @speedfl for the contribution!! And thanks @gsreynolds for the help! Teamwork!!! 🎉 👍 🌮

speedfl added 2 commits April 13, 2022 17:27

Issue 463: Introduce catch_all

d5db1ca

Add Delete feature

818a134

gsreynolds added 5 commits April 22, 2022 19:52

Add TestAccPagerDutyRulesetRule_CatchAllRule

b1b566f

Signed-off-by: Gavin Reynolds <[email protected]>

Add TestAccPagerDutyRulesetRule_CatchAllRuleRoute

0489eb5

Signed-off-by: Gavin Reynolds <[email protected]>

Reset all available actions in the catch_all rule if we "delete" it

6255a5a

Signed-off-by: Gavin Reynolds <[email protected]>

Don't omit when empty RuleActions, so we can "delete"/reset it to def…

e791254

…ault Signed-off-by: Gavin Reynolds <[email protected]>

Add catch_all docs for pagerduty_ruleset_rule

ec1d925

Signed-off-by: Gavin Reynolds <[email protected]>

gsreynolds mentioned this pull request Apr 22, 2022

Don't omit when empty RuleActions, so we can "delete"/reset it to default heimweh/go-pagerduty#84

Merged

speedfl added 3 commits April 27, 2022 14:09

Update dependencies

dffa6a8

Merge branch 'master' of github.com:speedfl/terraform-provider-pagerd…

5cc1363

…uty into bugfix/463

Fix np

8576530

stmcallister approved these changes Apr 29, 2022

View reviewed changes

stmcallister merged commit e53fd5c into PagerDuty:master Apr 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 463: Introduce catch_all #481

Issue 463: Introduce catch_all #481

speedfl commented Apr 13, 2022 •

edited

Loading

speedfl commented Apr 14, 2022 •

edited

Loading

speedfl commented Apr 20, 2022

gsreynolds commented Apr 22, 2022 •

edited

Loading

speedfl commented Apr 22, 2022

gsreynolds commented Apr 22, 2022

speedfl commented Apr 27, 2022

speedfl commented Apr 27, 2022

speedfl commented Apr 29, 2022

stmcallister left a comment

Issue 463: Introduce catch_all #481

Issue 463: Introduce catch_all #481

Conversation

speedfl commented Apr 13, 2022 • edited Loading

speedfl commented Apr 14, 2022 • edited Loading

speedfl commented Apr 20, 2022

gsreynolds commented Apr 22, 2022 • edited Loading

speedfl commented Apr 22, 2022

gsreynolds commented Apr 22, 2022

speedfl commented Apr 27, 2022

speedfl commented Apr 27, 2022

speedfl commented Apr 29, 2022

stmcallister left a comment

Choose a reason for hiding this comment

speedfl commented Apr 13, 2022 •

edited

Loading

speedfl commented Apr 14, 2022 •

edited

Loading

gsreynolds commented Apr 22, 2022 •

edited

Loading