thousandeyes / terraform-provider-thousandeyes

ThousandEyes Terraform Provider
Apache License 2.0
21 stars 26 forks source link

fix: Adding required ThousandEyes integration type #114

Closed bear-dev closed 1 year ago

bear-dev commented 1 year ago

Hello,

I've tried to setup ThousandEyes integrations and faced the issue:

2022-10-17T14:33:59.592-0600 [ERROR] vertex "thousandeyes_alert_rule.external_monitor_alert_rule" error: Missing required argument
2022-10-17T14:33:59.609-0600 [INFO]  backend/local: plan operation completed
╷
│ Error: Missing required argument
│
│   with thousandeyes_alert_rule.external_monitor_alert_rule,
│   on te_alert_rule.tf line 3, in resource "thousandeyes_alert_rule" "external_monitor_alert_rule":
│    3: resource "thousandeyes_alert_rule" "external_monitor_alert_rule" {
│
│ The argument "notifications.0.third_party.0.integration_type" is required,
│ but no definition was found.

I've added and successfully tested the missing argument.

...
  # thousandeyes_alert_rule.external_monitor_alert_rule will be created
  + resource "thousandeyes_alert_rule" "external_monitor_alert_rule" {
      + alert_rule_id             = (known after apply)
      + alert_type                = "Web Transactions"

      + notifications {
          + third_party {
              + integration_id   = "pgd-xxxx"
              + integration_type = "PAGER_DUTY"
            }
        }
    }
...
Plan: 1 to add, 0 to change, 0 to destroy.

Changes to Outputs:
  + pagerduty_integration_id   = "pgd-xxxx"
  + pagerduty_integration_type = "PAGER_DUTY"
...

Please review the proposed fix.

bear-dev commented 1 year ago

@pedro-te could you please review the fix. Thank you

sfreitas-te commented 1 year ago

@bear-dev thanks for your PR we will give feedback as soon as possible

bear-dev commented 1 year ago

So, I would suggest we use ResourceRead helper as I mentioned in the separate comment. What do you think?

I agree. I've verified the fix, and it works for me as well. Please check 91355f8cd0b1de5c098f0c87785399303c12f170

Regarding PagerDuty integration in general. It looks like auth_token and auth_user are not used anymore, and ThousandEyes API doesn't return anything. But I don't want to mix the integration check with additional changes. I'll create an additional PR for that. Do you agree?

pedro-te commented 1 year ago

So, I would suggest we use ResourceRead helper as I mentioned in the separate comment. What do you think?

I agree. I've verified the fix, and it works for me as well. Please check 91355f8

Regarding PagerDuty integration in general. It looks like auth_token and auth_user are not used anymore, and ThousandEyes API doesn't return anything. But I don't want to mix the integration check with additional changes. I'll create an additional PR for that. Do you agree?

Agreed, sounds good to me.

Thank you for your contribution once again, @bear-dev !