-
Notifications
You must be signed in to change notification settings - Fork 26
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
CHAOSPLT-316: Implement authentication in the http eventnotifier #865
CHAOSPLT-316: Implement authentication in the http eventnotifier #865
Conversation
Serialize the disruption and it into the payload of the http request sent by the HTTP notifier component.
Datadog ReportBranch report: ✅ 0 Failed, 738 Passed, 2 Skipped, 7m 14.46s Total Time |
…a.bissembayeva/CHAOSPLT-316/http-add-auth
b9b3a26
to
7dfa0d4
Compare
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.
+1 but I would like a second reviewer (i nominated Claire but it can be anyone)
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.
I added some minor comments
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.
You should add documentation on what this feature do here. (We should also definitely indicate that this feature ONLY support Authentication through a bearer token with a Authorization header!)
You could also add more comments in general I think.
Otherwise, 👍 nice one! I'll approve once the changes for documentation are made 👍
What does this PR do?
Please briefly describe your changes as well as the motivation behind them:
Code Quality Checklist
Testing
unit
tests orend-to-end
tests.unit
tests orend-to-end
tests.x