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

AA-1837 Add only_invocable_on_unresolved_incidents to Automation Action's schema. #945

Merged
merged 3 commits into from
Nov 13, 2024

Conversation

juliannagreen1
Copy link
Contributor

Adds new field only_invocable_on_unresolved_incidents to Automation Action's schema.

@juliannagreen1 juliannagreen1 marked this pull request as ready for review October 29, 2024 20:05
@juliannagreen1 juliannagreen1 changed the title Aa 1837 AA-1837 Add only_invocable_on_unresolved_incidents to Automation Action's schema. Oct 29, 2024
Copy link

@mtrudel mtrudel left a comment

Choose a reason for hiding this comment

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

LGTM though I'm no expert

@@ -214,7 +218,8 @@ resource "pagerduty_automation_actions_action" "foo" {
process_automation_job_id = "pa_job_id_123"
process_automation_job_arguments = "-arg 1"
process_automation_node_filter = "tags: production"
}
},
Copy link
Contributor

Choose a reason for hiding this comment

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

The trailing comma is breaking the test

@@ -75,6 +75,7 @@ resource "pagerduty_automation_actions_action" "test" {
process_automation_job_arguments = "-arg 1"
process_automation_node_filter = "tags: production"
}
only_invocable_on_unresolved_incidents: true
Copy link
Contributor

Choose a reason for hiding this comment

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

use = instead of :

@@ -298,6 +308,10 @@ func resourcePagerDutyAutomationActionsActionRead(d *schema.ResourceData, meta i
if automationActionsAction.ActionClassification != nil {
d.Set("action_classification", &automationActionsAction.ActionClassification)
}

if automationActionsAction.OnlyInvocableOnUnresolvedIncidents != nil {
d.Set("only_invocable_on_unresolved_incidents", &automationActionsAction.OnlyInvocableOnUnresolvedIncidents)
Copy link
Contributor

Choose a reason for hiding this comment

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

As automationActionsAction.OnlyInvocableOnUnresolvedIncidents is a pointer to bool the value should be its dereference (i.e. use * instead of &).

@@ -84,6 +85,7 @@ func TestAccPagerDutyAutomationActionsActionTypeProcessAutomation_Basic(t *testi
resource.TestCheckResourceAttrSet("pagerduty_automation_actions_action.foo", "modify_time"),
resource.TestCheckResourceAttrSet("pagerduty_automation_actions_action.foo", "runner_id"),
resource.TestCheckResourceAttr("pagerduty_automation_actions_action.foo", "runner_type", "runbook"),
resource.TestCheckResourceAttr("pagerduty_automation_actions_action.foo", "only_invocable_on_unresolved_incidents", "true"),
Copy link
Contributor

Choose a reason for hiding this comment

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

please, test the "false" value here setting explicitly the value in testAccCheckPagerDutyAutomationActionsActionTypeProcessAutomationConfigUpdated

@@ -153,6 +158,11 @@ func buildAutomationActionsActionStruct(d *schema.ResourceData) (*pagerduty.Auto
automationActionsAction.ModifyTime = &val
}

if attr, ok := d.GetOk("only_invocable_on_unresolved_incidents"); ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

GetOk will return ok equal to false even when the value is explicitly set to false. Since the client defines this field as omitempty it would never get changed to false.
I'd change this whole if block to:

attr, _ :=  d.Get("only_invocable_on_unresolved_incidents").(bool)
automationActionsAction.OnlyInvocableOnUnresolvedIncidents = &attr

@cjgajard cjgajard merged commit aa89132 into PagerDuty:master Nov 13, 2024
1 check passed
@cjgajard
Copy link
Contributor

Thank you for your contribution @juliannagreen1 😄

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