[ORCA-4601] Add support for Event Orchestration Cache Variables #822

c-kaieong · 2024-02-16T21:58:31Z

Changes

⚠️ These changes rely on updates to the Go Client, which will need to be released first: heimweh/go-pagerduty#149

In this PR, we are introducing Cache Variables resources for both Global Event Orchestrations and Service Event Orchestrations. As well, data sources and imports are supported.

⚠️ BEFORE MERGING

Update KB links in the following doc files:
- website/docs/d/event_orchestration_global_cache_variable.html.markdown
- website/docs/d/event_orchestration_service_cache_variable.html.markdown
- website/docs/r/event_orchestration_global_cache_variable.html.markdown
- website/docs/r/event_orchestration_service_cache_variable.html.markdown

Testing

Acceptance tests are passing locally

Further local testing performed outlined in: https://github.com/PagerDuty/terraform-provider-pagerduty-internal/pull/7

alenapan · 2024-02-20T19:42:33Z

pagerduty/event_orchestration_cache_variable_util.go

+			var diags diag.Diagnostics
+			min := 1
+			max := 84000
+
+			value := v.(int)
+			valid := value >= min && value <= max


Although it would be nice to immediately notify the user about the value being out of range, we looked into adding validation rules to the TF Provider in the past and decided against it mostly because we don't want to have validation logic distributed across multiple repos. And if it ever changes we'd need to make this change in two places, and the Provider changes usually take longer to go live because of the external dependencies.

Ah, that makes sense! I'll remove it

Addressed in 844a929

alenapan · 2024-02-20T19:56:17Z

pagerduty/event_orchestration_cache_variable_util.go

+	}
+
+	if oid == "" || id == "" {
+		return []*schema.ResourceData{}, fmt.Errorf("Error importing cache variable. Expected import ID format: <orchestration_id>:<cache_variable_id>")


We should probably display the format differently based on the cache variable type:

Global : "<orchestration_id>:<cache_variable_id>"

Service: "<service_id>:<cache_variable_id>"

Addressed in 844a929

alenapan · 2024-02-20T20:25:15Z

pagerduty/resource_pagerduty_event_orchestration_global_cache_variable.go

+		Schema: map[string]*schema.Schema{
+			"event_orchestration": {
+				Type:     schema.TypeString,
+				Required: true,
+				ForceNew: true,
+			},
+			"id": {
+				Type:     schema.TypeString,
+				Computed: true,
+			},
+			"name": {
+				Type:     schema.TypeString,
+				Required: true,
+			},
+			"condition": {
+				Type:     schema.TypeList,
+				Optional: true,
+				Elem: &schema.Resource{
+					Schema: resourceEventOrchestrationCacheVariableConditionSchema,
+				},
+			},
+			"configuration": {
+				Type:     schema.TypeList,
+				Required: true,
+				MaxItems: 1,
+				Elem: &schema.Resource{
+					Schema: resourceEventOrchestrationCacheVariableConfigurationSchema,
+				},
+			},
+			"disabled": {
+				Type:     schema.TypeBool,
+				Optional: true,
+			},
+		},


I don't have a strong opinion about this but wanted to surface it in case you prefer this approach. Since the only difference between global cache var and service cache var resource schemas is the "parent" (event_orchestration vs service), we can define a common, "base" schema, then extend this common schema with the corresponding "parent" property depending on the type. Here's an example of how we do it for EO actions: https://github.com/PagerDuty/terraform-provider-pagerduty/blob/master/pagerduty/resource_pagerduty_event_orchestration_path_service.go#L16C5-L90
Something like this:

var eventOrchestrationCacheVariableResourceSchema = map[string]*schema.Schema{ "id": { Type: schema.TypeString, Computed: true, }, "name": { Type: schema.TypeString, Required: true, }, "condition": { Type: schema.TypeList, Optional: true, Elem: &schema.Resource{ Schema: resourceEventOrchestrationCacheVariableConditionSchema, }, }, "configuration": { Type: schema.TypeList, Required: true, MaxItems: 1, Elem: &schema.Resource{ Schema: resourceEventOrchestrationCacheVariableConfigurationSchema, }, }, "disabled": { Type: schema.TypeBool, Optional: true, }, } var eventOrchestrationGlobalCacheVariableResourceSchema = buildEventOrchestrationGlobalCacheVariableResourceSchema() var eventOrchestrationServiceCacheVariableResourceSchema = buildEventOrchestrationServiceCacheVariableResourceSchema() func buildEventOrchestrationGlobalCacheVariableResourceSchema() { cv := eventOrchestrationCacheVariableResourceSchema cv["event_orchestration"] = &schema.Schema{ Type: schema.TypeString, Required: true, ForceNew: true, } return cv } func buildEventOrchestrationServiceCacheVariableResourceSchema() { ... }

And the same can be done for global and service cache var data source schemas.

Oh I actually wanted to do this from the very start, but couldn't figure out the syntax to accomplish it. I'll make the quick refactor, thanks!

I couldn't get the refactor going the way I wanted it. I'm just gonna leave this PR as-is instead of sinking more time into it 😅

alenapan · 2024-02-20T20:44:09Z

pagerduty/resource_pagerduty_event_orchestration_global_cache_variable_test.go

+			{
+				Config: testAccCheckPagerDutyEventOrchestrationGlobalCacheVariableConfig(orch, name1, orchn2, disabled1, config1, cond1),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckPagerDutyEventOrchestrationGlobalCacheVariableID(cv, orchn2),
+				),
+			},


So if I understand correctly, whenever the parent is updated, the ForceNew in the schema tells the provider to create a new resource. If that's the case, do you know what happens to the the cache var associated with the old parent? Does it get deleted from the TF state or do they both stay in the state and the next time they run terraform plan it shows that the old cache var will be deleted?

I guess I'm just trying to wrap my head around the scenario when someone creates a cache var for Event Orchestration A, then updates the parent to be Event Orchestration B, which creates a new variable on EO B. So the cache var on EO A is just kept in S3 and is not tracked by Terraform anymore?

I can test it locally too, but I thought I'd ask first in case you already tested it.

The ForceNew attribute forces the existing resource to be deleted then completely recreated from scratch. So under the hood, it'd be calling our DELETE API on OrchA, then just POST a new one on OrchB.

I've tested this locally, but doesn't hurt to have another pair of eyes.

Ah nice! I will test it locally too, but it's good to know that there no "leftover" resources after it was moved to a different parent.

Ok I'm running into some errors when trying to use a local provider so need to troubleshoot. But if you tested it locally and confirmed that a cache variable belonging to the old parent gets deleted I think we're good.

alenapan · 2024-02-20T20:51:29Z

pagerduty/data_source_pagerduty_event_orchestration_global_cache_variable.go

+}
+
+func dataSourcePagerDutyEventOrchestrationGlobalCacheVariableRead(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics {
+	return dataSourceEventOrchestrationCacheVariableRead(ctx, d, meta, pagerduty.CacheVariableTypeGlobal)


I think before sending this PR for review to José we'll need to point this branch to your branch of the API Client, so the checks can pass and the acceptance tests are passing locally. At least that's what we used to do in previous EO PRs, then after both PRs are reviewed/approved we'd merge the API Client and point this branch to the new version.

@c-kaieong not sure if the provider or internal docs have the instructions but this command should do it:

go mod edit -replace github.com/heimweh/go-pagerduty=github.com/c-kaieong/go-pagerduty@ORCA-4600-introduce-cache-variables && go mod tidy && go mod vendor

Thanks! The internal docs show how to point to a local directory, but not a branch. I'll add that in

https://github.com/PagerDuty/terraform-provider-pagerduty-internal/pull/8

alenapan

LGTM 👍

imjaroiswebdev

@c-kaieong before marking this as approved there are some comments to address please.

go.mod

pagerduty/event_orchestration_cache_variable_util.go

pagerduty/resource_pagerduty_event_orchestration_global_cache_variable_test.go

pagerduty/resource_pagerduty_event_orchestration_service_cache_variable_test.go

go.mod

c-kaieong force-pushed the ORCA-4601-add-support-for-cache-variables branch from 2d78c4a to 915502a Compare February 16, 2024 22:01

alenapan reviewed Feb 20, 2024

View reviewed changes

alenapan approved these changes Feb 20, 2024

View reviewed changes

c-kaieong force-pushed the ORCA-4601-add-support-for-cache-variables branch 2 times, most recently from 3b43f9d to 8be4a0f Compare March 11, 2024 20:38

c-kaieong marked this pull request as ready for review March 11, 2024 20:41

c-kaieong force-pushed the ORCA-4601-add-support-for-cache-variables branch from ca84de0 to e0d0a50 Compare March 21, 2024 20:37

imjaroiswebdev requested changes Mar 26, 2024

View reviewed changes

c-kaieong added 9 commits April 3, 2024 14:55

Add support for Cache Variables

5a52e41

Address PR comments

715a6d1

Add documentation for Cache Variables

1548a81

Change some reference syntax

246a50c

Update KB links

5398ecb

Rebase and point to dev branch again

ca69c98

Update to newer GO client version

0527854

Remove unneeded sleep calls and rename orch/service resources

24b7ea3

Update go client dependency

1727dfc

c-kaieong force-pushed the ORCA-4601-add-support-for-cache-variables branch from 5c8ef49 to 1727dfc Compare April 3, 2024 18:55

imjaroiswebdev approved these changes Apr 3, 2024

View reviewed changes

imjaroiswebdev merged commit a8a550f into PagerDuty:master Apr 3, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ORCA-4601] Add support for Event Orchestration Cache Variables #822

[ORCA-4601] Add support for Event Orchestration Cache Variables #822

c-kaieong commented Feb 16, 2024 •

edited

Loading

alenapan Feb 20, 2024

c-kaieong Feb 20, 2024

c-kaieong Feb 20, 2024

alenapan Feb 20, 2024

c-kaieong Feb 20, 2024

alenapan Feb 20, 2024

c-kaieong Feb 20, 2024

c-kaieong Feb 20, 2024

alenapan Feb 20, 2024

c-kaieong Feb 20, 2024

alenapan Feb 20, 2024

alenapan Feb 20, 2024

alenapan Feb 20, 2024

alenapan Feb 20, 2024

c-kaieong Feb 20, 2024

c-kaieong Feb 20, 2024

alenapan left a comment

imjaroiswebdev left a comment

[ORCA-4601] Add support for Event Orchestration Cache Variables #822

[ORCA-4601] Add support for Event Orchestration Cache Variables #822

Conversation

c-kaieong commented Feb 16, 2024 • edited Loading

Changes

⚠️ BEFORE MERGING

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alenapan left a comment

Choose a reason for hiding this comment

imjaroiswebdev left a comment

Choose a reason for hiding this comment

c-kaieong commented Feb 16, 2024 •

edited

Loading