-
Notifications
You must be signed in to change notification settings - Fork 68
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 Pagerduty trigger example in go #201
Add Pagerduty trigger example in go #201
Conversation
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.
Looks good Frankie, I will approve just in case and you can refer the comments afterwards.
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.
Thx Frankie, great contribution!
Once all comments are resolved, please sign and squash the commits. |
Signed-off-by: codegold79 <fgold@vmware.com>
d5c78fa
to
3f0116a
Compare
@embano1 I made the changes and put all the commits into one. I realized that I did that a little early as you haven't had a chance to re-review. Sorry about that. Hopefully, this function example is short/simple enough that looking through the code again won't be too much of a hassle. |
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.
Two minor things, but we can keep note of them in PR and change if required in a separate PR.
return []byte{}, fmt.Errorf("read http response body: %w", err) | ||
} | ||
|
||
if res.StatusCode < 200 || res.StatusCode >= 300 { |
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.
3xx does not necessarily have to be an error condition.
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.
Good point. I'll change that to not include 3xx.
} | ||
|
||
func pdSendRequest(ctx context.Context, path string, pdp pdPayload) ([]byte, error) { | ||
clt := &http.Client{} |
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.
It's best practice to create the client with some same timeouts.
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.
Definite yes to that suggestion. I'll add a timeout.
Signed-off-by: codegold79 <fgold@vmware.com>
2cc6196
to
1ff3a7e
Compare
|
||
reqBody, err := json.Marshal(pdp) | ||
if err != nil { | ||
return []byte{}, err |
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.
Nit: These returns could be simplified with return nil, err
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.
Left a comment (nit) which can be fixed in a future PR.
* Add go-pagerduty-trigger handle function example using Go Signed-off-by: codegold79 <fgold@vmware.com>
Summary
This PR adds a Go version of the PagerDuty trigger. Currently, there exists a version in Python.
Pull Request Checklist
🚨 Please review the guidelines for contributing to this repository.
WIP
keyword in the title of your PR if you are not ready for reviewChange Type
What types of changes does your code introduce to the VMware Event Broker Appliance?
Please check the type of change your PR introduces:
Resolved Issues
List of Issues closed or resolved by this PR. Add multiple
Closes
keyword followed by the issue number (e.g. Closes #ISSUE-NUMBER)Testing Verification
Additional Information
If you have any questions/comments, feel free to reach out to team on Slack #vcenter-event-broker-appliance
Thank you from the VEBA Team! 🥳