RFC: Process Github issue_comment events to support adhoc task creation

[bhearsum]

No description provided.

[djmitche]

Another thing to consider here is that we currently don't subscribe to issue-comment events. https://docs.taskcluster.net/docs/manual/deploying/github describes what we do subscribe to, and the "Pull Request" option has the following help text:

Pull request opened, closed, reopened, edited, assigned, unassigned, review requested, review request removed, labeled, unlabeled, synchronized, ready for review, converted to draft, locked, unlocked, auto merge enabled, auto merge disabled, milestoned, or demilestoned.

So, I think that listening for comment events will require changing that setting in the app, and last time we tried this that meant GitHub emailed all app users and asked them to re-authorize the app before allowing it to subscribe to those events. The app continues to work without the events until that re-authorization occurs. So, not a big problem, but something to be aware of for deployers.

[bhearsum]

So, I think that listening for comment events will require changing that setting in the app

This was my read, too.

and last time we tried this that meant GitHub emailed all app users and asked them to re-authorize the app before allowing it to subscribe to those events. The app continues to work without the events until that re-authorization occurs. So, not a big problem, but something to be aware of for deployers.

Good to know! Once this is available, it's a pretty big carrot so I suspect it'll be easy to get folks to re-authorize to gain access to the feature.

[hwine]

FYI: see changes to how GitHub is addressing this issue

[update 2021-05-07] This works differently than I thought at first -- this is a DoS via excessive PR prevention. Even when approved, PRs from forks do not have access to workflow secrets. The links on this page clarify that point.

[bhearsum]

FYI: see changes to how GitHub is addressing this issue

I saw that! It's a neat idea, and actually a bit better than what's proposed here, since it learns who is trusted over time. This proposal would require approval for every PR for the same contributor. We could extend to do something nicer like that later, I suspect.

[hwine]

This proposal would require approval for every PR for the same contributor.

Not as I read GitHub's proposal -- I assume it's in the context of a "normal" GitHub account, where there is a separate "maintainer" permissions group (above "write", below "admin"). So:

• "write" perms can say "do this one PR"
• "maintainer" perms can (only?) add user to allow list for future PRs

[edited to clarify antecedents. dang prepositions!]

[bhearsum]

This proposal would require approval for every PR for the same contributor.

Not as I read it -- I assume it's in the context of a "normal" GitHub account, where there is a separate "maintainer" permissions group (above "write", below "admin"). So:

* "write" perms can say "do this one PR"

* "maintainer" perms can (only?) add user to allow list for future PRs

Are you talking about this proposal, or Github's? If you're talking about Github's, I agree! If you're talking about this one, there's currently no plan in place to support an allow list (besides the existing mechanism of adding the user as a read collaborator).

[bhearsum]

It looks to me like this is pretty much settled. I just pushed a change to update the summary/motivation sections to match the more general implementation we ended up settling on, but I think this is ready for Final Comment. @imbstack, @petemoore - what do you think? (Please mark as Final Comment if you think it's ready, I don't have the ability to do so...)

[bhearsum]

I heard no objections in Final Comment. I opened https://bugzilla.mozilla.org/show_bug.cgi?id=1715848 for tracking implementation. @petemoore or @imbstack - can one of you merge this, please?