ci: add workflow for automated vote notification #1337
Conversation
|
/u |
|
Sample message: |
Shurtu-gal
requested review from
derberg,
asyncapi-bot-eve and
thulieblack
as code owners
|
/ptal |
|
@derberg @thulieblack Please take a look at this PR. Thanks! 👋 |
.github/workflows/vote-notify.yml
Outdated
| text: message, | ||
| }, { | ||
| headers: { | ||
| 'Authorization': `Bearer ${{ secrets.SLACK_TOKEN }}`, |
There was a problem hiding this comment.
we use existing one, no need to do anything here, right?
There was a problem hiding this comment.
Yeah, regarding this I don't know which secret has access to post messages. Need chat:write to be specific. For more info:
https://api.slack.com/methods/chat.postMessage
There was a problem hiding this comment.
yeah, there basically is no secret like SLACK_TOKEN - we just need to somehow create a dedicated slack token for that, this is a strategy we had so far, any publishing to slack, for any reason, always has dedicated token
There was a problem hiding this comment.
token under new app is under dedicated secret now: SLACK_DM_TSC with below scopes
There was a problem hiding this comment.
tested with below
curl -X POST "https://slack.com/api/chat.postMessage" \
-H "Authorization: Bearer NOTSOFASTSOLDIER" \
-H "Content-Type: application/json" \
-d '{
"channel": "U0572R8J927",
"text": "Hello, this is a test message from Lukasz Gornicki related to https://github.com/asyncapi/community/pull/1337!"
}'
did you get it @Shurtu-gal ?
There was a problem hiding this comment.
@Shurtu-gal left few comments, thanks a lot!
I think it is ok to make first version based on slack, and in the meantime we need to discuss with TSC if they feel fine sharing emails openly in maintainers.yml
Co-authored-by: Lukasz Gornicki <[email protected]>
.github/workflows/vote-notify.yml
Outdated
|
|
||
| // Sending Slack DM via API | ||
| const response = await axios.post('https://slack.com/api/chat.postMessage', { | ||
| channel: member.slack, |
There was a problem hiding this comment.
you need to either set a proper error handling here, or just check it before calling API
slack is added manually to MAINTAINERS.yaml - there is no way to get the slack id in smart automated way. So there might be situation that we overlooked it and some TSC member do not have this information provided
- we need to validate if slack property is added - and also have error handling in case wrong slack id is provided and API call fails
- in both cases we need to learn about that issue and drop notification to proper channel using
SLACK_CI_FAIL_NOTIFYsecret - you will find examples of how we do it in other workflows
There was a problem hiding this comment.
Sure, give me some time I will do this as well.
There was a problem hiding this comment.
@derberg, sorry to take so long. I have added it.
Shurtu-gal
changed the title
Signed-off-by: Ashish Padhy <[email protected]>
|
@derberg sorry for stretching this out. I have made the requested changes:
|
|
|
||
| - name: Notify TSC Members | ||
| # if: steps.list.outputs.result.length > 0 | ||
| uses: actions/github-script@v7 |
There was a problem hiding this comment.
sorry but with my experience in automation maintainance, especially of workflows with long scripts
- can you put slack notification in separate function, with proper large comment, and also pretty large comment where it is executed - so if we add other notification channels - for example using nodemailer to send email
- pleas also make other functions if possible, so the main script reads like a flow of steps
- part of the code where we determine in notification should go out, should also be separated and easy to configure - like every 3 days or only 3 days before deadline (and easy to configure to 5) for example. You know what I mean
also wondering how we can test it before merging 🤔
Description
Related issue(s)
Closes #1163