[Incident][Alert] Add From Parameter #338
Conversation
| } | ||
|
|
||
| // ManageIncidentAlertsWithContext allows you to manage the alerts of an incident. | ||
| func (c *Client) ManageIncidentAlertsWithContext(ctx context.Context, incidentID string, alerts *IncidentAlertList) (*ListAlertsResponse, error) { | ||
| lar, _, err := c.manageIncidentAlertsWithContext(context.Background(), incidentID, alerts) | ||
| func (c *Client) ManageIncidentAlertsWithContext(ctx context.Context, from string, incidentID string, alerts *IncidentAlertList) (*ListAlertsResponse, error) { |
There was a problem hiding this comment.
Could you add to the GoDoc comment of both of these methods some context around the from parameter? Maybe something like // The from parameter is the email address of a valid user, associated with the PagerDuty account, making the request.
| func (c *Client) manageIncidentAlertsWithContext(ctx context.Context, incidentID string, alerts *IncidentAlertList) (*ListAlertsResponse, *http.Response, error) { | ||
| resp, err := c.put(ctx, "/incidents/"+incidentID+"/alerts/", alerts, nil) | ||
| func (c *Client) manageIncidentAlertsWithContext(ctx context.Context, from string, incidentID string, alerts *IncidentAlertList) (*ListAlertsResponse, *http.Response, error) { | ||
|
|
There was a problem hiding this comment.
Could you remove this line of whitespace from the top of the function, so h := ... is right up against the header?
| @@ -649,7 +649,7 @@ func TestIncident_ManageAlerts(t *testing.T) { | |||
| }, | |||
| }, | |||
| } | |||
| res, _, err := client.ManageIncidentAlerts(incidentID, input) | |||
| res, _, err := client.ManageIncidentAlerts(from, incidentID, input) | |||
There was a problem hiding this comment.
This doesn't seem to actually assert that the from value is making it through as an HTTP header. Could you update the handler above (on line 635) to look for that header and the correct value, and fail the request/test otherwise?
| "From": from, | ||
| } | ||
|
|
||
| resp, err := c.put(ctx, "/incidents/"+incidentID+"/alerts/", alerts, h) |
There was a problem hiding this comment.
I know this existed before you made the change, but it seems like the trailing / on /alerts/ is a mistake. Could you remove it so that it just reads "/alerts"?
|
Thank you for the contribution! There are a few opportunities to improve this PR, and so I added some comments with my feedback/thoughts. Let me know if you have any questions. |
|
@tuannvm thank you again for this contribution. I wanted to reach back out to see if you had a moment to address the comments I made above, as well as whether you can resolve the merge conflicts and then rebase/squash your commit(s)? The |
|
@tuannvm I wanted to ping one last time to see if you may want to help shepherd this PR forward. Because I can't easily merge my own PRs, I would appreciate any time you could spare to help get this ready to be merged. If not I totally understand, and appreciate you raising this PR. |
|
I opened #380 to supersede this PR. |
Why?
fromparameterfromheader is mandatory