slack-welcomer: add workflow-only moderation for #kubernetes-careers#78
slack-welcomer: add workflow-only moderation for #kubernetes-careers#78Champbreed wants to merge 3 commits into
Conversation
k8s-ci-robot
added
the
sig/contributor-experience
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Champbreed The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
k8s-ci-robot
added
cncf-cla: yes
BenTheElder
left a comment
BenTheElder
left a comment
There was a problem hiding this comment.
Thanks, some first pass comments [keep in mind I haven't worked with the slack API in a while and I have leveraged AI here]
Is it intended that we block thread replies in addition to direct messages?
| ) | ||
|
|
||
| var guardedChannels = map[string]bool{ | ||
| "C25QWBR71": true, // #kubernetes-careers |
There was a problem hiding this comment.
Should probably be configurable? I think there are other instances of this application (CNCF?)
May also make this testable against another instance.
| switch e := err.(type) { | ||
| case slack.ErrRateLimit: | ||
| log.Printf("Slack is rate limiting us, trying again in %s...\n", e.Wait) | ||
| time.Sleep(e.Wait) |
There was a problem hiding this comment.
We should maybe backoff with a maximum timeout. This can hang indefinitely currently (competing calls keep exhausting the rate limit)
| if err != nil { | ||
| log.Fatalf("Failed to load config from %s: %v", o.configPath, err) | ||
| } | ||
| adminToken, err := loadAdminToken(o.configPath) |
There was a problem hiding this comment.
Can we make this part of the main config instead of reading in configPath twice?
|
@kubernetes-sigs/slack-infra-admins |
|
Thanks for the review, @BenTheElder. I've updated the code so we only parse the config file once to cut out that double disk read, moved the guarded channels into a dynamic slice so other instances like CNCF can use it and we can test it locally, capped the deletion retry loop at 5 tries so the execution thread doesn't hang indefinitely under heavy rate-limiting, and added fast-failing validation on startup to stop the app from crashing silently later, all while keeping the broadcast-only behavior since blocking direct human thread replies is completely intentional for this workflow-driven channel unless we need to revisit this later. PTAL. |
|
Sorry I just don't have much time at the moment and I'm going to be out for a bit. Please poke the slack admins chat to see if anyone else can help in the interim. |
|
I think I need to step down from this repo. I'm over extended and this doesn't overlap with the other things I'm doing at all. |
|
@BenTheElder Thanks for your review. |
3 participants
This PR adds message moderation to the
slack-welcomerapp so it can automatically remove direct posts in#kubernetes-careersand enforce the channel's workflow rules.Changes
adminToken) alongside the normal bot token so the app has permission to delete messages.team_joinandmessageevents, added logic to catch and delete unapproved direct posts while messaging the user, and included a back-off retry loop to handle Slack rate limits gracefully./sig contributor-experience
cc: @jberkus @mrbobbytables