Experiment: CODENOTIFY #155440
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| name: Ping maintainers | ||
|
|
||
| on: | ||
| pull_request_target: | ||
| types: [opened, synchronize, ready_for_review] | ||
| push: | ||
| types: [opened, synchronize, ready_for_review] | ||
|
|
||
| jobs: | ||
| codenotify: | ||
| name: codenotify | ||
| runs-on: ubuntu-latest | ||
| # Don't notify people in forks os nixpkgs | ||
| if: github.repository_owner == 'NixOS' | ||
| steps: | ||
| - uses: actions/checkout@v2 | ||
| with: | ||
| # Already include changes in the PR for pinging | ||
| ref: refs/pull/${{ github.event.pull_request.number }}/merge | ||
| - uses: sourcegraph/[email protected] | ||
| env: | ||
| GITHUB_TOKEN: ${{ secrets.CODENOTIFY_GITHUB_TOKEN }} | ||
|
There was a problem hiding this comment. I thought I had responded to this but GitHub does not show it anymore. Did you get anything? There was a problem hiding this comment. It needs to be set by an admin. There was a problem hiding this comment.
Ah, sorry I misread this. No I didn't see a response. There was a problem hiding this comment. Interesting. Anyways, I don't know what I'm doing there and why, I mostly copied some example configuration. If I understand it correctly, the token is required to be able to ping teams. So it should be able to ping individuals even without. |
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| ** @mweinelt |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
How does this action work with
pushevents ?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 know. I figured that I wanted to re-run the action on every push for testing the initial setup. I guess that it'll comment on the PR after every push, but we'll have to find out
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.
No.
pushis for stuff that happens outside of PRs. Can't request a review of a commit pushed to master.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.
Does this action request reviews or post comments?
ofborgused to post comments and it was changed to review requests, IIRC people thought it was too spammy.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.
Oh, I understand. In that case,
pushis indeed kinda useless.The action posts comments instead of requesting reviews (by design, see their README). There's a lot of configuring to be done, but I wanted to start simple. (Configuring as in "forking modifying the action", since it has no YAML options.)
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.
Posting comments may be a dealbreaker TBH.
Review requests are preferred as they can't be resent, posting/updating comments will notify every time.
IIRC there was another bot before
ofborgthat also posted comments, might be worthwhile checking what happened to it?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 agree that we want review requests. Since the action does not have a lot of code, this shouldn't be too hard to add. But we first need to decide whether we want this at all, or if we prefer some self-written custom solution of some sorts.
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.
Having looked at it a bit more I don't think using
sourcegraph/codenotifyis a good idea.IMHO allowing
CODENOTIFYfiles at arbitrary places in the tree that can't be easily inspected without tooling is a negative considering the size of our tree and number of contributors.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 can always restrict ourselves to using only the top-level entry first and then discuss that issue later on. IMHO the experiences with large files (all-packages.nix, release notes, …) have taught that they are rather ugly to work with and distributing content from a central file to the place it actually refers to is a good idea.
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.
How?
If it can't be restricted via tooling to the top-level file it needs to be discussed first.
Currently maintainers could be set via
meta.maintainersorCODEOWNERS. It is in a predictable position, either at the top or the bottom of the tree. It can be easily inspected manually or formeta.maintainersit can be queried it usingnix.With this approach it can be set anywhere, can't be easily inspected manually and it can only be queried via a custom tool?