Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CODEOWNERS with list of maintainers #7439

Closed
wants to merge 3 commits into from
Closed

Conversation

stokito
Copy link
Contributor

@stokito stokito commented Dec 1, 2024

The second attempt of the #6830 that was closed after a discussion.
My proposition is to add the CODEOWNERS file to help for contributors to quickly find a maintainer (as I did myself today). Even better, for those devs that already have a write access they'll automatically be assigned as reviewers for a PR for the particular app.

The other steps like creating luci-dev team as I described in my comment can be done later. But the CODEOWNERS can start helping us today and save time for contributors. We may add a link to it into the PR template.

It won't change anything in terms of security so the PR is safe to merge.

@Ramon00
Copy link
Contributor

Ramon00 commented Dec 14, 2024

just merge this?

@hnyman
Copy link
Contributor

hnyman commented Dec 14, 2024

Not much has changed since #6830 which did not receive approval.
(and like jow noted in that PR, the proposed list already contains quite many ancient users who haven't been active for a while)

@stokito
Copy link
Contributor Author

stokito commented Dec 14, 2024

I'm not sure why that PR was closed. The discussion turned into enabling an automatic reviewer assignment. To enable this it needs to configure a team. Here I recreated the PR because even without this step this is just a list of github nicknames of maintainers. If some maintainer is not active anymore we can remove it from the list and from the package maintainer field. Anyway if someone creates a PR they'll find same persons and CC them. But we can simplify this step.

@Ramon00
Copy link
Contributor

Ramon00 commented Dec 15, 2024

I guess this is in the catagory "better than nothing". But anyway, somebody needs to make decision, either merge or not. Waiting for the all singing and dancing solution will likely mean nothing will happen.

The CODEOWNERS will request for a review automatically by a path.

Signed-off-by: Sergey Ponomarev <[email protected]>
Signed-off-by: Sergey Ponomarev <[email protected]>
@systemcrash
Copy link
Contributor

I've read earlier revisions of info on this document and found some things left unexplained. More recent revisions seem much more clear.

https://github.com/github/docs/blob/main/content/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-code-owners.md

But... unless a user mentioned in the CODEOWNERS file has write permissions to this repo, it's nothing more than a glorified reference document (so no behaviour changes will result, at present standards; a good thing). The document needs to be maintained, however. Do you commit to maintaining it?

@stokito
Copy link
Contributor Author

stokito commented Dec 31, 2024

no problem, please merge

@stokito
Copy link
Contributor Author

stokito commented Dec 31, 2024

I also added the link to CODEOWNERS to PR template. Maybe it also worth to add a link to issues but we may probably overwhelm maintainers. We better to do a triage ourselves and mention a maintainer if needed.

@stokito
Copy link
Contributor Author

stokito commented Jan 7, 2025

@systemcrash friendly remind on this

@hnyman
Copy link
Contributor

hnyman commented Jan 7, 2025

@stokito
You keep pushing for this, but so far I have seen no green light from any of the core OpenWrt developers who should have a say, neither in this PR or in the other (where there was hesitancy and opposition).

Well, looking for more closely:
Actually Github makes this easy for us: Apparently currently you can't be made as CODEOWNERS list member without write access. This PR with visible error messages shows it quite clearly:

image

image

And as there should be no commit access given to that amount of people, this is an automatic no-go.

@hnyman hnyman closed this Jan 7, 2025
@stokito
Copy link
Contributor Author

stokito commented Jan 7, 2025 via email

@systemcrash
Copy link
Contributor

So... I don't think having this is bad or causes any undesirable behaviour changes. Githubs documentation on the feature is now much more clear on it. It just requires being kept up to date, something, which I think @stokito seems OK with maintaining?

@stokito
Copy link
Contributor Author

stokito commented Jan 9, 2025 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants