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

[ORCA-4600] Add support for cache variables #149

Merged

Conversation

c-kaieong
Copy link
Contributor

Add CRUD support for Event Orchestration Cache Variables

type EventOrchestrationCacheVariable struct {
ID string `json:"id,omitempty"`
Name string `json:"name,omitempty"`
Disabled bool `json:"disabled,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

I think omitempty needs to be removed from the Disabled property because it drops "falsy" fields from the payload. And here is what's classified as a falsy field, according to the docs:

The "omitempty" option specifies that the field should be omitted from the encoding if the field has an empty value, defined as false, 0, a nil pointer, a nil interface value, and any empty array, slice, map, or string.

So basically if they want to send disabled: false they won't be able to do it. We had a similar issue with EOs where a customer created a Router with disabled rules first, then tried setting them to disabled: false, and the new values kept getting ignored because they were never sent to the API.

ID string `json:"id,omitempty"`
Name string `json:"name,omitempty"`
Disabled bool `json:"disabled,omitempty"`
Conditions []*EventOrchestrationCacheVariableCondition `json:"conditions,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

And same with Conditions - because we want to be able to send empty arrays we should remove omitempty.

}
}

func TestGlobalOrchestrationICacheVariableUpdate(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo in TestGlobalOrchestrationICacheVariableUpdate

Suggested change
func TestGlobalOrchestrationICacheVariableUpdate(t *testing.T) {
func TestGlobalOrchestrationCacheVariableUpdate(t *testing.T) {

}
}

func TestGlobalOrchestrationIntegrationDelete(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo in TestGlobalOrchestrationIntegrationDelete

Suggested change
func TestGlobalOrchestrationIntegrationDelete(t *testing.T) {
func TestGlobalOrchestrationCacheVariableDelete(t *testing.T) {

setup()
defer teardown()

oId := "a64f9c87-6adc-4f89-a64c-2fdd8cba4639"
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Let's maybe change the oId here and in all service Cache Var tests below to a valid obfuscated service ID to reflect the real world scenario more accurately?

}
}

func TestServiceOrchestrationICacheVariableUpdate(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo in TestServiceOrchestrationICacheVariableUpdate

Suggested change
func TestServiceOrchestrationICacheVariableUpdate(t *testing.T) {
func TestServiceOrchestrationCacheVariableUpdate(t *testing.T) {

}
}

func TestServiceOrchestrationIntegrationDelete(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo in TestServiceOrchestrationIntegrationDelete

Suggested change
func TestServiceOrchestrationIntegrationDelete(t *testing.T) {
func TestServiceOrchestrationCacheVariableDelete(t *testing.T) {

@c-kaieong
Copy link
Contributor Author

@alenapan Ah, thanks for catching all those subtle typos and the clarification around omitempty! I've made the changes in 5279d06

Copy link
Contributor

@alenapan alenapan left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Probably worth it to leave it as a draft PR since we can't merge this until the Public API docs go live

@c-kaieong c-kaieong force-pushed the ORCA-4600-introduce-cache-variables branch from 5279d06 to c11bc4a Compare March 11, 2024 19:11
@c-kaieong c-kaieong marked this pull request as ready for review March 11, 2024 20:26
Copy link
Collaborator

@imjaroiswebdev imjaroiswebdev left a comment

Choose a reason for hiding this comment

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

@c-kaieong there are a couple of comments to address before moving forward with merge/approval. Other than that, good job 👍🏽

pagerduty/event_orchestration_cache_variable.go Outdated Show resolved Hide resolved
pagerduty/event_orchestration_cache_variable.go Outdated Show resolved Hide resolved
@imjaroiswebdev
Copy link
Collaborator

LGTM 👍 Probably worth it to leave it as a draft PR since we can't merge this until the Public API docs go live

I'd also prefer to leave this as a Draft PR in the meantime.

@metavida
Copy link
Contributor

metavida commented Apr 2, 2024

@imjaroiswebdev I just now published pubic REST API docs that are used by this API. For example: https://developer.pagerduty.com/api-reference/a87e8f7117af9-create-a-cache-variable-for-a-global-event-orchestration

The Cache Variables feature in Event Orchestration is now officially GA for all customers so we're ready to merge this PR!

@imjaroiswebdev imjaroiswebdev self-requested a review April 3, 2024 15:07
Copy link
Collaborator

@imjaroiswebdev imjaroiswebdev left a comment

Choose a reason for hiding this comment

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

Great! We are ready to merge now 💪🏽

@imjaroiswebdev imjaroiswebdev merged commit 5876af2 into heimweh:master Apr 3, 2024
1 check passed
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.

4 participants