ci: add workflow to enable voting #1120
Conversation
AayushSaini101
requested review from
quetzalliwrites,
derberg,
asyncapi-bot-eve and
thulieblack
as code owners
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
AayushSaini101
changed the title
AayushSaini101
changed the title
|
Testing of Workflow: AayushSaini101/Vote#14 |
There was a problem hiding this comment.
you're duplicating verification of TSC member.
please move that step into a reusable simple action like this https://github.com/asyncapi/.github/blob/master/.github/actions/get-node-version-from-package-lock/action.yml in .github repo
you can see how it is used in https://github.com/asyncapi/.github/blob/master/.github/workflows/if-nodejs-release.yml#L49
we will use this action more ofter, so better to move it into .github as reusable action now.
regarding docs -> please create a new VOTING.md document
.github/workflows/add_vote_label.yml
Outdated
| steps: | ||
| - uses: actions/checkout@v3 | ||
|
|
||
| - name: Get the comment body |
There was a problem hiding this comment.
Removed @derberg thanks
.gitvote.yml
Outdated
| allowed_voters: | ||
| teams: | ||
| - tsc_members | ||
| users: |
There was a problem hiding this comment.
all of them should be in tsc_members so no need to explicitly list users
There was a problem hiding this comment.
Added new yaml file
.github/workflows/add_vote_label.yml
Outdated
| with: | ||
| script: | | ||
| const fs = require('fs'); | ||
| const filePath = 'TSC_MEMBERS.json'; |
There was a problem hiding this comment.
wrong file, your source of truth is https://github.com/asyncapi/community/blob/master/MAINTAINERS.yaml and entries with isTscMember set to true
also when you will be refactoring your code, please do not use old callbacks but do modern code with async/await
There was a problem hiding this comment.
Changed thanks @derberg
.github/workflows/add_vote_label.yml
Outdated
| } | ||
| }); | ||
|
|
||
| - name: Add the label |
There was a problem hiding this comment.
we also need a step that will post a comment back in case not TSC member added /vote, to put a message that only TSC members can do it
here you have example how to do it: https://github.com/asyncapi/.github/blob/master/.github/workflows/bounty-program-commands.yml#L33
There was a problem hiding this comment.
@derberg when we allowed only TSC members voting, this will be double check, The user will be filter out when the it is not matching with the authorised voters, this will be message.
AayushSaini101/Vote#15 (comment)
Which is not a proper message by a gitvote-bot, we can allow voting for all contributors, then we can provide the custom comment not default one, This would be the best way to describe the situation in comments, instead of using default comments of the particular vote, What do you think ? @derberg
There was a problem hiding this comment.
The representation of the templates of the gitvote is not proper for unauthorised vote https://github.com/cncf/gitvote/tree/main/templates, We can modify easily and make proper by changing own workflow as you shared in this https://github.com/asyncapi/.github/blob/master/.github/workflows/bounty-program-commands.yml#L33
There was a problem hiding this comment.
This will be the message when unauthorised author commented:
There was a problem hiding this comment.
oh yeah, if git vote bot does the verification for us, then we should use it and not do it on our own as we will the over communicate, duplicate it kinda
There was a problem hiding this comment.
We cannot test the /vote command in the individual account need to use in the community, Reference cncf/gitvote#480
- i removed)%20%26%26%20env) the authorised for the /vote command, it will be handled by gitvote workflow
- The /cancel-vote command authorisation will be handled by our workflow not by bot one, Reference: sdf AayushSaini101/Vote#29 (comment)
There was a problem hiding this comment.
we also need a step that will post a comment back in case not TSC member added
/vote, to put a message that only TSC members can do ithere you have example how to do it: https://github.com/asyncapi/.github/blob/master/.github/workflows/bounty-program-commands.yml#L33
Thanks @derberg Done AayushSaini101/Vote#29 (comment) Testing
AayushSaini101
force-pushed
the
Issue-1093
branch
from
40d2fb9 to
04c7047
Compare
AayushSaini101
force-pushed
the
Issue-1093
branch
from
8168f84 to
2aa27ae
Compare
AayushSaini101
force-pushed
the
Issue-1093
branch
from
fd0a604 to
b5b28f6
Compare
There was a problem hiding this comment.
remember to ping me @AayushSaini101 in case you are ready for another review round
Yes @derberg please review I have one concern regarding the /vote command added comment above |
|
why do we need |
@derberg I want to test whether the .gitvote.yml is authorising the tsc_members from yml file or not. I cannot test on my person repo. cncf/gitvote#480 Reference: I just need to check whether it is allowing voting or not in this way. |
|
@AayushSaini101 ok, but I guess we can assume that their native functionality works, right? |
yes, we have to check only for the /vote command. |
| @@ -0,0 +1,44 @@ | |||
| name: Verify Member | |||
There was a problem hiding this comment.
| name: Verify Member | |
| name: Verify TSC Member |
There was a problem hiding this comment.
also please change the folder name
There was a problem hiding this comment.
Done @derberg
| name: Verify Member | ||
| outputs: | ||
| isTSCMember: | ||
| description: 'Verify Member' |
There was a problem hiding this comment.
I guess we should specify it is true/false
There was a problem hiding this comment.
done @derberg
| value: ${{steps.verify_member.outputs.isTSCMember}} | ||
| inputs: | ||
| authorName: | ||
| description: 'Name of the Author' |
There was a problem hiding this comment.
actually github handle not the author name
There was a problem hiding this comment.
Done @derberg change the description
| using: "composite" | ||
| steps: | ||
| - name: Checkout repository | ||
| uses: actions/checkout@v2 |
There was a problem hiding this comment.
Done @derberg
| uses: actions/checkout@v2 | ||
|
|
||
| - name: Install the dependencies | ||
| run: npm install js-yaml |
There was a problem hiding this comment.
please install by specifying version
There was a problem hiding this comment.
Done @derberg
|
|
||
| - name: Verify TSC Member | ||
| id: verify_member | ||
| uses: actions/github-script@v6 |
There was a problem hiding this comment.
Done @derberg
| // Load YAML file | ||
| const data = yaml.load(fs.readFileSync('MAINTAINERS.yaml', 'utf8')); | ||
| // Iterate over each person object | ||
| data.forEach(person => { |
There was a problem hiding this comment.
chat gpt can give you a better alternative with .filter
There was a problem hiding this comment.
Done @derberg
| // Iterate over each person object | ||
| data.forEach(person => { | ||
| // Check if the person is a TSC member or not | ||
| if (person.isTscMember && person.github == commenterName) { |
There was a problem hiding this comment.
person.isTscMember - we cannot assume we always get boolean, can be that true or false is a string, and then this will always return true, even if "false"
There was a problem hiding this comment.
Done @derberg Added condition more
Co-authored-by: Lukasz Gornicki <[email protected]>
Co-authored-by: Lukasz Gornicki <[email protected]>
There was a problem hiding this comment.
left few comments related to docs
removed screenshots as in such cases they are risky and take time to maintain - you can see they are for example already outdated for our case.
if you think they were useful, then better maybe link to some example PR from other project where it was used - as a side note - cncf/toc#1263 (comment)
Co-authored-by: Lukasz Gornicki <[email protected]>
Co-authored-by: Lukasz Gornicki <[email protected]>
Co-authored-by: Lukasz Gornicki <[email protected]>
Co-authored-by: Lukasz Gornicki <[email protected]>
Co-authored-by: Lukasz Gornicki <[email protected]>
|
AayushSaini101/Vote#52 Testing of the changes |
|
There was a problem hiding this comment.
@AayushSaini101 lgtm
now, I have the following proposal
because with this PR we are not only adding automation, but we also change the way voting works (excluding discussions for example)
this means we need to ask TSC if they approve
and this means we could ask for vote and actually test how voting works at the same time
my proposal:
- split this PR in 2
pr Aonly with GH actions and gitvote config. We merge without asking anyone - so we have it in master ready for final testpr Bwith documentation only. onceAis merged, I put a vote comment with explanation about what are we voting on
thoughts?
|
ah, and if you agree, please treat this pr as |
@derberg Sounds good, Let's check :) |
@derberg File removed, |
|
LGTM for anyone looking at this one - we merge without asking for approval as this is they only way to test - any changes will later be applied through another PR that will be used for voting and also testing automation /rtm |
|
@asyncapi/bounty_team |
Resolve: #1093