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 crier RocketChat reporter #30135
base: master
Are you sure you want to change the base?
Conversation
Welcome @robinschneider! |
Hi @robinschneider. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/cc |
Thanks! I'm not sure how much I'll be able to look at this immediately but I'll try to not let it rot ;) |
* Fix lint issues * Fix prow-config-documented.yaml
/retest |
@robinschneider: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@petr-muller Thank you! I fixed the lint issues. |
/ok-to-test |
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'm not a great fan of essentially duplicating this much code, but let's defer asking for some kind of WebhookReporter abstraction to anyone who tries to add a third copy ;)
Some comments inline.
* Improve rocketchat reporter * Reduce code duplication * Moved test to clientutil
@petr-muller Thank you again for your feedback. |
@petr-muller I wanted to check in and see if you've had a chance to review the recent changes I made. Your insights and feedback would be greatly appreciated. Please let me know if you have any thoughts or suggestions. |
Sorry, I switch between "vacation" and "catching up with accumulated backlog" modes in summer :) I hope to get to k/test-infra reviews this week again. |
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.
Thank you!
oh right, this changes the CRD |
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 don't have a lot of time to review OSS PRs, unfortunately, so I didn't read the entire PR. However (1) I am not opposed to this (we're just adding a new reporter type), and (2) it seems like Petr already reviewed some things, so I think that's good enough. I just have some minor questions, thanks.
|
||
// This is necessary since RocketChat is optional. | ||
if o.rocketChatChannel != "" { | ||
tokens = append(tokens, o.rocketChatChannel) |
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'm not super familiar with Crier, but what stops us from adopting o.rocketChat.Channel
, similar to o.bugzilla.ApiKeyPath
? I.e., we should probably group the rocketChat-related things into o.rocketChat
, no?
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 am not familiar with the bugzilla
reporter, but my implementation is a copy of the slack
reporter that does it the same way. Hope this is fine.
New changes are detected. LGTM label has been removed. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: petr-muller, robinschneider The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
@robinschneider: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Hello @robinschneider, the Prow source code has been moved to https://github.com/kubernetes-sigs/prow on April 9, 2024. |
Add crier reporter for RocketChat.
Reporter is based on the Slack reporter.
It uses a RocketChat Webhook.
Docs will be contributed to the Documentation repository.