docs(contribute): update contributing guides

[aali309]

Welcome to Cryostat! 👋

Before contributing, make sure you have:

• Read the contributing guidelines
• Linked a relevant issue which this PR resolves
• Linked any other relevant issues, PR's, or documentation, if any
• Resolved all conflicts, if any
• Rebased your branch PR on top of the latest upstream main branch
• Attached at least one of the following labels to the PR: [chore, ci, docs, feat, fix, test]
• Signed all commits using a GPG signature

To recreate commits with GPG signature git fetch upstream && git rebase --force --gpg-sign upstream/main

---

Fixes: #1665

Description of the change:

updated CONTRIBUTING.md

[aali309]

Did not add the /build_test part as requested by @tthvo since it only applies to the reviewers team

[mwangggg]

I thought if someone opened a PR and got the reviewers team to add the safe-to-test label, they could comment /build_test?

[aali309]

I thought if someone opened a PR and got the reviewers team to add the safe-to-test label, they could comment /build_test?

if that's the case, I should add the /build_test on the doc. I just thought only reviewers could utilize \build_test ? @andrewazores

[andrewazores]

https://github.com/cryostatio/cryostat/blob/02041456d91e6b7b67f7254e5e072a6784e3b239/.github/workflows/pr-ci.yml#L32

Here's the definition for what fails the /build_test command.

[andrewazores]

!(github.event.comment.author_association == 'MEMBER' || github.event.comment.author_association == 'OWNER')

The commenter must have author association of "member" or "owner":

https://docs.github.com/en/graphql/reference/enums#commentauthorassociation

Which means they are part of the org or own the repo - it's not clear to me from this if that means they own the upstream repo or if they own the fork that the PR was opened from. Perhaps it can be an external author here.

github.event.issue.user.name != github.event.comment.user.name

This clause looks like it restricts the command to the PR's author, but perhaps I'm misunderstanding.

[aali309]

!(github.event.comment.author_association == 'MEMBER' || github.event.comment.author_association == 'OWNER')

The commenter must have author association of "member" or "owner":

https://docs.github.com/en/graphql/reference/enums#commentauthorassociation

Which means they are part of the org or own the repo - it's not clear to me from this if that means they own the upstream repo or if they own the fork that the PR was opened from. Perhaps it can be an external author here.

github.event.issue.user.name != github.event.comment.user.name

This clause looks like it restricts the command to the PR's author, but perhaps I'm misunderstanding.

Yes, it appears the same to me. The OWNER is the one owner of the forked repo that the PR was created from not the cryostat repo owner. If this is the case then @mwangggg is right.

[aali309]

Contradicting my previous understanding.. After reading, what I understood is that the term "OWNER" typically refers to the owner of the upstream repository where the actions are being triggered. Unfortunately, could not find clear documentation to clarify this. So I think the github.event.comment.author_association == 'OWNER' condition typically checks the permissions and associations of the owner of the upstream repository, not the person who opened the PR.

[mwangggg]

yea I thought it was the github.event.issue.user.name != github.event.comment.user.name line that checks if the person who opened the PR is the same user that commented.. but perhaps I just implemented that incorrectly

[tthvo]

yea I thought it was the github.event.issue.user.name != github.event.comment.user.name line that checks if the person who opened the PR is the same user that commented.. but perhaps I just implemented that incorrectly

Looks like its correct tho. Here is the doc about issue_comment payload: https://docs.github.com/en/webhooks/webhook-events-and-payloads#issue_comment

And since the comment is made on upstream repo, I believe github.event.comment.author_association represents association with upstream.

[andrewazores]

I got distracted from this. So what conclusion do we come to - the /build_test works for any PR author, so long as it has been labelled safe-to-test (which requires a reviewer to do)? I think that sounds like what we had originally intended, so if that's true then it makes sense to document.

[aali309]

I got distracted from this. So what conclusion do we come to - the /build_test works for any PR author, so long as it has been labelled safe-to-test (which requires a reviewer to do)? I think that sounds like what we had originally intended, so if that's true then it makes sense to document.

No, I think the build_test only works for reviewers, so need to document it.

[andrewazores]

Okay, that's fine too. Maybe down the line it'll be worth trying to update that to allow any author so long as the PR has been labelled, but I think that's pretty low priority. On the other hand, maybe that ability should be locked down to only reviewers or at least members of the contributors team - people we recognize and already trust.