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

Create Incident, List Priorities, and headers in POST method support #122

Merged
merged 10 commits into from
Jun 29, 2018

Conversation

ldelossa
Copy link

@ldelossa ldelossa commented Jun 6, 2018

Submitting this PR which adds support for POSTing (creating) an incident along with GETing (listing) priorities in the new priorities feature. Willing to change around any struct naming or json construction logic at request.

Methods added:

client.CreateIncidents
client.ListPriorities

PR also plumbed header support into POST method on the client.

@ldelossa ldelossa force-pushed the incident-priorities-support branch from a5a2cdb to 51d7663 Compare June 6, 2018 22:43
@ldelossa ldelossa changed the title Incident priorities support Create Incident, List Priorities, and headers in POST method support Jun 6, 2018
@wdhnl
Copy link
Contributor

wdhnl commented Jun 7, 2018

I think the method name should be CreateIncident not CreateIncidents since it creates a singular incident

The naming of the type to pass into the CreateIncident should be CreateIncidentOptions to match other method signatures

@wdhnl
Copy link
Contributor

wdhnl commented Jun 7, 2018

Since CreateIncidentResponse isn't returned anywhere (just used to unmarshal the response inside CreateIncident(s) ), should it be un-exported?

(I struggled with same thing on my PR)

@ldelossa
Copy link
Author

ldelossa commented Jun 7, 2018

@wdhnl agree to both your comments, will update.

@wdhnl
Copy link
Contributor

wdhnl commented Jun 19, 2018

Will you have time to update this?

@ldelossa
Copy link
Author

@wdhnl dunzo. sorry about that got side tracked.

@wdhnl
Copy link
Contributor

wdhnl commented Jun 20, 2018

@mattstratton can you evaluate this PR?

@ldelossa
Copy link
Author

ldelossa commented Jun 20, 2018 via email

@ldelossa
Copy link
Author

@wdhnl renamed CreateIncidentValue to CreateIncidentOptions like suggested.

@wdhnl
Copy link
Contributor

wdhnl commented Jun 21, 2018

awesome, thanks

@mattstratton mattstratton merged commit 5f93c38 into PagerDuty:master Jun 29, 2018
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.

3 participants