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

Add support for Service Event Rules #304

Merged
merged 9 commits into from
Mar 10, 2021

Conversation

mrzacarias
Copy link
Contributor

@mrzacarias mrzacarias commented Mar 4, 2021

Adding support for Service Event Rules. API reference:
List Service's Event Rules
Get an Event Rule from a Service
Delete an Event Rule from a Service
Create an Event Rule on a Service
Update an Event Rule on a Service

Some other changes:

  • Added the "suspend" rule action to keep the rule payload up to date with the API.
  • Adding Service Rule List and Service Rule Show CLI commands
  • Stubbing other possible Service Rule CLI commands

Edit: Forgot to mention, but I got trouble making the CLI logic work without changing the references to our own fork, so I commented the code for now. As soon as we merge this PR, we can have the implemented CLI commands working by uncommenting this code.

Marcio Rocha Zacarias and others added 6 commits March 2, 2021 16:05
* Adding "suspend" action to rule actions

* Adding Service Event Rules support
* General updates, adding service rule list cli command

* Adding service rule show command
@theckman theckman added this to the v1.4.0 milestone Mar 5, 2021
@theckman
Copy link
Collaborator

theckman commented Mar 5, 2021

@mrzacarias I'll get to reviewing this in the coming days, once I finish a set of PRs I am raising against the repo. Thank you for the contribution.

Copy link
Collaborator

@theckman theckman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey there, I've some other PRs in flight that are making some fairly large changes to the internals of this package. Unfortunately, your PR is overlapping with some of those and so I've requested changes to have this code align more closely to what changes I am making there. That includes things like accepting a context.Context, no longer returning a *http.Response from API methods, and only using pointer types where they make sense for semantic reasons.

Please let me know if you have any questions about this feedback. 👍

service.go Outdated Show resolved Hide resolved
service.go Outdated Show resolved Hide resolved
service.go Outdated Show resolved Hide resolved
service.go Outdated Show resolved Hide resolved
service.go Outdated Show resolved Hide resolved
service.go Outdated Show resolved Hide resolved
service.go Show resolved Hide resolved
command/service_rule_list.go Outdated Show resolved Hide resolved
ruleset.go Show resolved Hide resolved
service.go Outdated Show resolved Hide resolved
@mrzacarias mrzacarias force-pushed the mz_merge_to_upstream branch from 196939d to 2fd77dc Compare March 8, 2021 18:45
@mrzacarias
Copy link
Contributor Author

mrzacarias commented Mar 8, 2021

@theckman Thanks for carefully reviewing this PR :)

Copy link
Collaborator

@theckman theckman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mrzacarias I think I may have found two more minor things that didn't dawn on me before. Since changing the types would be a breaking change after release, I want to make sure we get this right.

Happy to be wrong with my requested changes, if so give me a shout.

service.go Outdated Show resolved Hide resolved
ruleset.go Outdated Show resolved Hide resolved
@theckman theckman merged commit 0a55cfc into PagerDuty:master Mar 10, 2021
@theckman
Copy link
Collaborator

@mrzacarias thank you for the contribution. There are still a few changes in-flight for v1.4.0, and so it make take a few more days (maybe weeks) until those get buttoned up. Once they do you'll be part of that version.

Thanks again!

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.

2 participants