-
Notifications
You must be signed in to change notification settings - Fork 243
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
Add json field incidents_responders to Incident struct #365
Add json field incidents_responders to Incident struct #365
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sostakas Thank you for the contribution. I did add one comment on the PR requesting a change.
Secondarily, according to the PagerDuty API documentation that field is not present when getting an incident: https://developer.pagerduty.com/api-reference/reference/REST/openapiv3.json/paths/~1incidents~1%7Bid%7D/get
Could you point me to the documentation where it shows that field being returned? We tend to only support fields that are explicitly documented by PagerDuty.
@@ -89,6 +89,7 @@ type Incident struct { | |||
Body IncidentBody `json:"body,omitempty"` | |||
IsMergeable bool `json:"is_mergeable,omitempty"` | |||
ConferenceBridge *ConferenceBridge `json:"conference_bridge,omitempty"` | |||
IncidentResponders []IncidentResponders `json:"incidents_responders,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you implement a test that ensures this field is being passed along in the JSON within the request body?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@theckman Thanks for comment. I added the test. It is not documented but you can check response body using Try it
. This field is must to test if additional responders were added successfully (if you want to stick to go-pagerduty and do not want to create your own http requests)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for that context. The policy set by PagerDuty on this library is that only fields that are explicitly listed in the documentation are merged in.
I'll label this PR as being focused on an undocumented feature, and try to engage with PagerDuty to see if they'd be willing/able to document the field(s). I'm not an employee, so I'll probably end up filing a support ticket to make the request to have the documentation updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@theckman Thank you!
…body in TestIncident_Get
@@ -237,15 +237,15 @@ func TestIncident_Get(t *testing.T) { | |||
|
|||
mux.HandleFunc("/incidents/1", func(w http.ResponseWriter, r *http.Request) { | |||
testMethod(t, r, "GET") | |||
_, _ = w.Write([]byte(`{"incident": {"id": "1"}}`)) | |||
_, _ = w.Write([]byte(`{"incident": {"id": "1", "incidents_responders": [{"incident": {"id": "1"}}]}}`)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for doing this. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add json:"incidents_responders,omitempty" to Incident struct in order to get
"incident": {"incidents_responders": []} from func GetIncident(ctx context.Context, client *pagerduty.Client, id string) (*pagerduty.Incident, error)