Add CODEOWNERS and fix missing PKG_MAINTAINER #6830
Conversation
|
In order to make it working the users needs to have a write access: You can open the file directly and GitHub will show a warning for the missing perms: |
|
Probably |
|
I can take a look at the work flow. But I think for code owner things, we
need to loop in @aparcar a d see whether path permissions are doable or
even desirable. Hitherto things have worked but finding github handles is
sometimes tricky.
… Probably treewide: Add CODEOWNERS and fix missing PKG_MAINTAINER would be
a subject which can pass CI checks.
|
|
The PR can be merged and start working today. Please note that the PR also adds missing maintainers for packages when not specified. @aparcar friendly reminder on this. |
|
I added owners for the new luci-app-cloudflared. Can we merge the PR before it stalled? :) |
|
Please merge the PR. It will help to find a maintainer and tag it on PRs. |
|
Undecided. My hunch is that this will simply rot, it already contains quite a number of contributors who have not been active for a very long time. I'm not keen on handing everyone on this list above write access either. |
|
Currently when creating a PR a contributor needs to check a history of a Makefile to find a username of author to ask him for a review. Even just having a text file with list would simplify this step significantly. I agree that the list may become outdated but fortunately it's not that fast and easy to fix. As a more advanced solution we can make a github workflow that will lookup a github username by a PKG_MAINTAINER and assign a reviewer. The the only source of thuth will be the Makefile itself. Still it would be just easier to edit the GITHUBOWNERS. |
Can we limit this to only active contributors with existing write access? |
|
yes, this is how it works. The CODEOWNERS doesn't grant perms by itself. |
The CODEOWNERS will request for a reviewer automatically by a path. https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-code-owners The luci-app-bcp38 had a maintainer from luci-app-acme and that looks like a copy-paste error. So it's maintainer was changed to an author @danrl Daniel F. Dickinson changed email address to <dfdpublic@wildtechgarden.ca> luci-all-lxl has a maintainer Petar Koretic <petar.koretic@sartura.hr> but there is no corresponding GitHub account. So Dirk Brenken was added as a second maintainer: he answered on an issue of the app. When maintainer wasn't set the initial author was used, or most contributor or Jo-Philipp Wich as a default. If anyone can't be a maintainer it should create an issue and the CODEOWNERS file will be updated with a new one. Signed-off-by: Sergey Ponomarev <stokito@gmail.com>
Let's dare to move forward. Each openwrt repo needs more reviewers and maintainers who have flexibility to make changes. Perhaps start with a handful, and augment as necessary. Pick ten names from that list who have committed in the last few years. Start with @stokito for the apps he's brought in. Another thing I noticed on re-reading the help text: |
|
giving more people commit access somewhat means one evil maintainer could hijack all packages since the build process isn't separating between package builds. Therefore I'm strongly against a more relaxed commit policy until we have either sandboxed or incremental builds. @jow- what do you think which solution is more feasible? |
Code owners limits any committer to specified folders. |
|
Wrt. @aparcar objections, the single folder restriction does not help as each app folder can inject arbitrary build logic through makefiles. |
I could fire a laser through space and slowly evaporate the moon. But I don't think I'm going to be doing that today. This topic will be revisited in the future. Closing... |
|
We need to create a Then go to the repo Settings / Rules/ Rulesets Create a Ruleset Ruleset Name: main Now a code owner e.g. an app maintainer can merge PRs into master only for parts that owns. Here I made a demo (sorry for echo, didn't noticed but have work now) Once you decide to use the feature you can grab the commits from the branch |
The CODEOWNERS will request for a reviewer automatically by a path.
https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-code-owners
The luci-app-bcp38 had a maintainer from luci-app-acme and that looks like a copy-paste error. So it's maintainer was changed to an author @danrl
Daniel F. Dickinson changed email address to dfdpublic@wildtechgarden.ca
luci-all-lxl has a maintainer Petar Koretic petar.koretic@sartura.hr but there is no corresponding GitHub account. So Dirk Brenken was added as a second maintainer: he answered on an issue of the app.
When maintainer wasn't set the initial author was used, or most contributor or Jo-Philipp Wich as a default.
If anyone can't be a maintainer he or she should create an issue and the CODEOWNERS file will be updated with a new one.