Issue 463: Introduce catch_all

[speedfl]

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]

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]

Any news ?

[gsreynolds]

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]

Thanks a lot for your help @gsreynolds !

[gsreynolds]

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

[speedfl]

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

[speedfl]

@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 = "geoffrey.muselli@test.com"
}

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]

@gsreynolds any news ? :)

[stmcallister]

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