Experiment: CODENOTIFY #155440
Experiment: CODENOTIFY #155440
Conversation
piegamesde
force-pushed
the
codeowners
branch
from
d766a40 to
d78b543
Compare
| on: | ||
| pull_request: | ||
| types: [opened, synchronize, ready_for_review] | ||
| push: |
There was a problem hiding this comment.
How does this action work with push events ?
There was a problem hiding this comment.
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.
I guess that it'll comment on the PR after every push, but we'll have to find out
No. push is 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.
Does this action request reviews or post comments?
ofborg used to post comments and it was changed to review requests, IIRC people thought it was too spammy.
There was a problem hiding this comment.
Oh, I understand. In that case, push is 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.
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 ofborg that also posted comments, might be worthwhile checking what happened to it?
There was a problem hiding this comment.
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.
Having looked at it a bit more I don't think using sourcegraph/codenotify is a good idea.
https://github.com/sourcegraph/codenotify#why-use-codenotify
There can be a CODENOTIFY file in any directory.
IMHO allowing CODENOTIFY files 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.
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.
We can always restrict ourselves to using only the top-level entry first then discuss that issue later on.
How?
If it can't be restricted via tooling to the top-level file it needs to be discussed first.
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.
Currently maintainers could be set via meta.maintainers or CODEOWNERS. It is in a predictable position, either at the top or the bottom of the tree. It can be easily inspected manually or for meta.maintainers it can be queried it using nix.
With this approach it can be set anywhere, can't be easily inspected manually and it can only be queried via a custom tool?
| ref: ${{ github.event.pull_request.head.sha }} | ||
| - 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.
I thought I had responded to this but GitHub does not show it anymore. Did you get anything?
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.
ofborg
bot
added
10.rebuild-darwin: 0
piegamesde
force-pushed
the
codeowners
branch
from
d78b543 to
4b268b0
Compare
|
@zowoq Thank you for having a look. As you can tell, I don't really know a lot about GitHub actions and I'm just pressing buttons and see what they do ^^. |
stale
bot
added
the
2.status: stale
FliegendeWurst
added
the
6.topic: continuous integration
stale
bot
removed
the
2.status: stale
|
What's the status of this given #347610 (new codeowners mechanism) was merged? |
Motivation for this change
A first step towards #143441. Just an experiment
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)nixos/doc/manual/md-to-db.shto update generated release notes