-
Notifications
You must be signed in to change notification settings - Fork 242
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 support for v2 ManageEvent api call #103
Conversation
d7d3534
to
37d4d42
Compare
Can we get this merged? |
I would also like see if this can be merged. Looks fine to me. Also why is the library stating that it is supporting the V2 API, when in fact it is not? This seems a very essential feature to me. |
I have no idea who is in charge, but this library sadly seems to be pretty much abandoned by pagerduty. I think a new project should be created that is autogenerated based on the swagger api definition for the v2 api. This would make keeping up to date much easier. |
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 the patience in waiting for the review.
Please address the comments in the PR and we will be happy to take another look.
// Event includes the incident/alert details | ||
type V2Event struct { | ||
RoutingKey string `json:"routing_key"` | ||
Action string `json:"event_action"` |
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.
Please rename this to EventAction
for consistency with the JSON field name
} | ||
|
||
// Response is the json response body for an event | ||
type V2EventResponse struct { |
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.
Based on our current API documentation for the Events API v2, the response format is different than what is expected here.
return nil, err | ||
} | ||
defer resp.Body.Close() | ||
if resp.StatusCode != http.StatusAccepted { |
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.
The API also returns validation errors back, it'll be great to have those displayed back to the user.
I haven't dug into this PR enough yet, but is it backwards compatible with v1? If not, we will want to be very clear with this release that it contains breaking changes. I am intending to add binary release options via goreleaser shortly; we should wait to merge this until we have the major versions cut, which I hope to do within the next couple days. |
@mattstratton Hey! Would it be possible to add this soon? Since the struct names contain |
I took the liberty of adding the spike by @tmccleve from #83 to my fork in order to support creation of v2 API events. The code is almost identically to the v1 code that is already in the package apart from the JSON structures and the endpoint url.