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

Set up a few code owners #40925

Merged
merged 4 commits into from
May 23, 2021
Merged

Set up a few code owners #40925

merged 4 commits into from
May 23, 2021

Conversation

DilumAluthge
Copy link
Member

No description provided.

@DilumAluthge
Copy link
Member Author

DilumAluthge commented May 23, 2021

Any committer should feel free to add themselves as code owners for their areas of expertise - just make a PR to edit the .github/CODEOWNERS file.

Here's the syntax for the CODEOWNERS file: https://docs.github.com/en/github/creating-cloning-and-archiving-repositories/creating-a-repository-on-github/about-code-owners#codeowners-syntax

@ViralBShah
Copy link
Member

Is it possible to create teams of people, and then delegate parts of the codebase to the different teams in the CODEOWNERS?

@DilumAluthge DilumAluthge merged commit 3bbe22e into master May 23, 2021
@DilumAluthge DilumAluthge deleted the dpa/codeowners branch May 23, 2021 13:02
@DilumAluthge
Copy link
Member Author

Is it possible to create teams of people, and then delegate parts of the codebase to the different teams in the CODEOWNERS?

Definitely!

/src/ @JuliaLang/compiler
/stdlib/LinearAlgebra/ @JuliaLang/linearalgebra
/stdlib/libblastrampoline_jll/ @staticfloat
/test/ @JuliaLang/test
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure this is all that useful since almost any PR adds some tests.

Copy link
Member

Choose a reason for hiding this comment

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

My thought was to see how useful this feature is and get it calibrated.

/.github/workflows/ @JuliaLang/github-actions
/.github/ @JuliaLang/github-actions
/.buildkite/ @JuliaLang/github-actions
/src/ @JuliaLang/compiler
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps this should be a bit more fine-grained? I'd be comfortable reviewing frontend changes, but likely won't be able to contribute much if it touches codegen or parts of the C API. Also should perhaps add base/compiler?

Copy link
Member

Choose a reason for hiding this comment

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

What do you think of my suggestion in #40925 (comment)

Copy link
Member

@simeonschaub simeonschaub May 23, 2021

Choose a reason for hiding this comment

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

Yeah, maybe. Although especially for some files in base this might be a bit of a noisy measure and it might be better for folks to be able to opt in manually.

Copy link
Member

Choose a reason for hiding this comment

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

Won't people's global preferences on notifications anyways override this?

Copy link
Member

Choose a reason for hiding this comment

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

I meant noisy here mostly in terms of that it might be rather arbitrary who is assigned to a file or not, but perhaps that's something we should just try and then see how it goes.

@ViralBShah
Copy link
Member

ViralBShah commented May 23, 2021

Maybe we should generate this file automatically based on the interesection set of git blame authors and which of those are committers on julialang. And then maybe pick top 3.

@DilumAluthge
Copy link
Member Author

Also, note that later lines in the CODEOWNERS file override earlier lines. So e.g. if you have this:

*       @foo 
*.js    @bar 

If you have a PR that only modifies .js files, then @bar will be pinged, but @foo will not be pinged.

@DilumAluthge
Copy link
Member Author

Maybe we should generate this file automatically based on the interesection set of git blame authors and which of those are committers on julialang. And then maybe pick top 3.

I think this would work well.

And we can always manually edit the result. So e.g. someone can choose to remove themselves from the file if they don't like getting the pings.

@DilumAluthge
Copy link
Member Author

For now, we've pared down a bit: #40929

shirodkara pushed a commit to shirodkara/julia that referenced this pull request Jun 9, 2021
Co-authored-by: Viral B. Shah <ViralBShah@users.noreply.github.com>
johanmon pushed a commit to johanmon/julia that referenced this pull request Jul 5, 2021
Co-authored-by: Viral B. Shah <ViralBShah@users.noreply.github.com>
@DilumAluthge DilumAluthge added backport 1.6 Change should be backported to release-1.6 backport 1.7 ci Continuous integration labels Aug 25, 2021
@KristofferC KristofferC mentioned this pull request Aug 25, 2021
75 tasks
KristofferC pushed a commit that referenced this pull request Aug 25, 2021
Co-authored-by: Viral B. Shah <ViralBShah@users.noreply.github.com>
(cherry picked from commit 3bbe22e)
KristofferC pushed a commit that referenced this pull request Aug 31, 2021
Co-authored-by: Viral B. Shah <ViralBShah@users.noreply.github.com>
(cherry picked from commit 3bbe22e)
KristofferC pushed a commit that referenced this pull request Sep 3, 2021
Co-authored-by: Viral B. Shah <ViralBShah@users.noreply.github.com>
(cherry picked from commit 3bbe22e)
@KristofferC KristofferC removed backport 1.6 Change should be backported to release-1.6 backport 1.7 labels Sep 7, 2021
staticfloat pushed a commit that referenced this pull request Dec 23, 2022
Co-authored-by: Viral B. Shah <ViralBShah@users.noreply.github.com>
(cherry picked from commit 3bbe22e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Continuous integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants