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

add CODEOWNERS #7277

Merged
merged 2 commits into from
Dec 5, 2022
Merged

add CODEOWNERS #7277

merged 2 commits into from
Dec 5, 2022

Conversation

fricklerhandwerk
Copy link
Contributor

as a supplement to #7229 as discussed on the Nix team

@edolstra @thufschmitt feel free to add more specifics. Also, who else has merge access and may want to get notified?

Copy link
Member

@thufschmitt thufschmitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 from me (although I wouldn't merge it without an agreement from the whole Nix team).

In a "small steps" fashion, I'll refrain from making any addition to the file itself, leaving that for follow-up PRs

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/tweag-nix-dev-update-39/23130/1

.github/CODEOWNERS Outdated Show resolved Hide resolved
@fricklerhandwerk
Copy link
Contributor Author

Discussed on Nix team meeting 2022-11-11:

  • @edolstra: this seems to do a subset of the auto-assign bot's work. let's make sure that works first. no point having two different files denoting responsibilities
    • @thufschmitt: if the bot requires reviewers to be collaborators too, let's stick to CODEOWNERS
    • @edolstra: will try the bot with someone who has no commit access

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2022-11-11-nix-team-meeting-notes-7/23451/1

@fricklerhandwerk
Copy link
Contributor Author

@edolstra does the review assignment workflow work already? The test PR still fails, but I got assigned to #3600 by some GitHub Action.

@edolstra
Copy link
Member

It seems to work sometimes. On the test PR it fails with "Resource not accessible by integration" whatever that means.

@thufschmitt
Copy link
Member

I'd suspect it works for PRs that come from the NixOS/nix repo and fail for the others

@fricklerhandwerk
Copy link
Contributor Author

fricklerhandwerk commented Nov 30, 2022

I that case it's useless, since everyone except maintainers will make PRs from their forks.

PS: Actually I think everyone including maintainers should do this, to keep only branches which serve a clear purpose so they are discoverable.

@thufschmitt
Copy link
Member

I that case it's useless, since everyone except maintainers will make PRs from their forks.

Yes, totally agreed

PS: Actually I think everyone including maintainers should do this, to keep only branches which serve a clear purpose so they are discoverable.

I don't think that's too problematic. I agree that using the fork as the base branch is a good default, but it's occasionally useful to create PRs directly from the NixOS repo. For stacked PRs in particular, there's still no good way to do them if all the branches aren't local.

@thufschmitt thufschmitt merged commit 3b05124 into NixOS:master Dec 5, 2022
@fricklerhandwerk
Copy link
Contributor Author

Discussed in the Nix team meeting on 2022-12-05:

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2022-12-05-nix-team-meeting-minutes-14/23836/1

@fricklerhandwerk fricklerhandwerk deleted the codeowners branch January 3, 2023 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants