-
Notifications
You must be signed in to change notification settings - Fork 93
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
#98 support for API v2 #111
Conversation
Move some responses in separate file
LO-1004-1007
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.
@biggless Thank you so much for putting this together, and it will be a big help for folks using the package and PagerDuty. I personally no longer have access to a PD account for testing, so my review focuses on some general release-ability notes. I'm going to trust that you have it working for your team, and therefore is ready for the general population.
Can you update the README.md
to swap out the HUBOT_PAGERDUTY_SUBDOMAIN
for HUBOT_PAGERDUTY_FROM_EMAIL
as well as some text explaining the change? This will be a major
version bump, so hopefully folks will read the release notes.
Also I guess the test suite didn't complain about any of these changes? We probably can go ahead and bump the .travis.yml
file so that it runs more recent versions of Node. See here for the suggested versions.
/cc @silverp1 , as he was working on something prior and may want to collaborate on this one. |
Going to do a bit more cleanup on some documentation, but this is 👍 |
There are a lot of changes and we did not test it yet. But it should be available for some code review 👻