PagerDuty / go-pagerduty

go client library for PagerDuty v2 API
https://v2.developer.pagerduty.com/docs/rest-api
Apache License 2.0
287 stars 241 forks source link

Chaining GetService and UpdateService fails when AlertGroupingParameters is omitted #438

Closed mjlshen closed 2 years ago

mjlshen commented 2 years ago
service, err := pd.GetService(serviceID, nil)
if err != nil {
    return err
}

service.status = "disabled"

if _, err = pd.UpdateService(*service); err != nil {
    return err
}

Results in the following when the following error when AlertGroupingParameters is unset/omitted by Go:

"error":"HTTP response failed with status code 400, message: Invalid Input Provided (code: 2001): Alert grouping parameters is invalid."

The JSON response from GetService looks like:

"alert_grouping_parameters": {
      "type": null,
      "config": null
    },

but the resulting struct ends up like, which the PagerDuty API doesn't seem to like:

pd.AlertGroupingParameters{
    Type:   "",
    Config: pd.AlertGroupParamsConfig{
        Timeout:   0,
        Aggregate: "",
        Fields:    nil,
    },
}
mjlshen commented 2 years ago

As a workaround, a check like this works before calling UpdateService

if service.AlertGroupingParameters.Type == "" {
     service.AlertGroupingParameters = nil
}
theckman commented 2 years ago

I'm not quite sure if there's anything we can do here. Because the field is a pointer, it should be nil if the PagerDuty response omits the object. But as you've shown the API isn't omitting the object, even though all fields are null, which feels like a bug on their side.

I help maintain this library in my spare time to benefit the Go community, but am not employed there. This means I don't have any special access to engineering, so I'm curious if you're able to file a bug report with PagerDuty support?

mjlshen commented 2 years ago

Appreciate the response! I'll see what I can do and report back here

mjlshen commented 2 years ago

Didn't have much luck with support, but with a closer look I do think it is a bug in this library where it's missing a couple omitempty's, so I made that PR^

mjlshen commented 2 years ago

@theckman when you get a chance, I would appreciate a review on my proposed fix - thanks!