-
-
Notifications
You must be signed in to change notification settings - Fork 109
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 workflow for automated vote notification #1337
base: master
Are you sure you want to change the base?
Conversation
/u |
/ptal |
@derberg @thulieblack Please take a look at this PR. Thanks! 👋 |
id: list | ||
with: | ||
script: | | ||
const { data: issues } = await github.rest.issues.listForRepo({ |
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.
will it list PRs as well?
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 think so does gitvote
allow PR voting as well?
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.
Facing a problem with PRs though, getting filtered results by vote-open
label is not possible. Should I go ahead with manually filtering the whole list of PRs.
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.
yeah, you either do it "manually"
const { data: pullRequests } = await github.pulls.list({
owner,
repo: repoName,
state: 'open'
});
const prsWithLabel = pullRequests.filter(pr =>
pr.labels.some(label => label.name === 'vote-open')
);
or use search API
const { data } = await github.search.issuesAndPullRequests({
q: `repo:${owner}/${repoName} is:pr is:open label:"vote-open"`,
});
I personally have bad experience with search and its accuracy, so I guess manual is better
- name: Fetch Current State | ||
id: fetch | ||
run: | | ||
cd .vote_state |
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.
where it comes from?
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.
A branch stores the data between workflow runs https://github.com/ash17290/asyncapi-community/tree/vote_state
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.
ok, please extend description then, for future generations :D
text: message, | ||
}, { | ||
headers: { | ||
'Authorization': `Bearer ${{ secrets.SLACK_TOKEN }}`, |
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.
we use existing one, no need to do anything here, right?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 <lpgornicki@gmail.com>
|
||
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_NOTIFY
secret - you will find examples of how we do it in other workflows
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.
Sure, give me some time I will do this as well.
Description
Related issue(s)
Closes #1163