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

feat: add http webhook #5233

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

zendesk-piotrpawluk
Copy link

what

Adding HTTP webhook implementation to send notifications to any HTTP(S) endpoint.

  • added http webhook kind
  • added --webhook-http-headers optional parameter to pass any extra headers with the request
  • added ProjectName to webhook payload
  • updated webhooks documentation

why

To send notifications to other systems than just Slack. Feature request: #810

tests

  • I have tested my changes by ...
    • unit tests (added new)
    • performing applies inside our testing environment

references

@zendesk-piotrpawluk zendesk-piotrpawluk requested review from a team as code owners January 13, 2025 12:32
@zendesk-piotrpawluk zendesk-piotrpawluk requested review from GenPage, nitrocode and X-Guardian and removed request for a team January 13, 2025 12:32
@github-actions github-actions bot added docs Documentation go Pull requests that update Go code website labels Jan 13, 2025
@dosubot dosubot bot added the feature New functionality/enhancement label Jan 13, 2025
Signed-off-by: Piotr Pawluk <piotr.pawluk@zendesk.com>
@zendesk-piotrpawluk
Copy link
Author

Rebased

@jamengual jamengual added the waiting-on-review Waiting for a review from a maintainer label Jan 19, 2025
Copy link
Contributor

@lukemassa lukemassa left a comment

Choose a reason for hiding this comment

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

Nice! I left a few small comments but overall nice addition!

}

// RoundTrip handles each http request.
func (t *AuthedTransport) RoundTrip(req *http.Request) (*http.Response, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

https://pkg.go.dev/net/http#RoundTripper says

RoundTrip should not modify the request, except for consuming and closing the Request's Body
I'm not very familiar with the http package, so I'm not sure what the implications are if the request is modified (or if adding headers qualifies as "modifying"), any thoughts here?

As an alternative you could always pass the headers into the webhook itself then set them on doSend()

return nil
}

func (h *HttpWebhook) doSend(_ logging.SimpleLogging, applyResult ApplyResult) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since doSend() is a non-exported method (i.e. unlike Send()) you should be able to omit logging.SimpleLogging altogether instead of passing it in and not using it.

(Though this does mean you'll have to make logging.SimpleLogging _ in Send())

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jan 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation feature New functionality/enhancement go Pull requests that update Go code lgtm This PR has been approved by a maintainer waiting-on-review Waiting for a review from a maintainer website
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature reqeuest: Generic webhook for notifications, instead of slack-token
3 participants