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

Create CODEOWNERS #12

Merged
merged 7 commits into from
Apr 1, 2021
Merged

Create CODEOWNERS #12

merged 7 commits into from
Apr 1, 2021

Conversation

lalitb
Copy link
Member

@lalitb lalitb commented Mar 30, 2021

As of now, it's individual aliases in the CODEOWNERS file. I have taken liberty to take current approvers from core repo, along with @seemk and @TomRoSystems and make them default code owner. We can start adding component level owners once the repo grows.

Please raise a comment here if someone doesn't want to be in list, or would like to add someone here, or want to be owner of only specific component (currently nginx & httpd ) within repo ?

Once we all agree, we can raise request to create approver group accordingly. Also whether we need to have separate approvers and maintainers, or let one of the default approvers be responsible to merge the changes once we have sufficient approvals.

I can't find @seemk from Reviewers search list ( not sure why ) - Requesting him to please review, or if someone can add him as reviewer.

# https://help.github.com/en/articles/about-code-owners
#

* @ThomsonTan @seemk @TomRoSystems @jsuereth @maxgolov @lalitb @pyohannes
Copy link
Member

Choose a reason for hiding this comment

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

Can we add something like @open-telemetry/cpp-contrib-approvers?

Copy link
Member Author

@lalitb lalitb Mar 30, 2021

Choose a reason for hiding this comment

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

We don't have this team for now. I will raise request to create one for this repo. Meanwhile we can discuss in this PR if all in the list agree to be there :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Have added this group now.

Copy link
Contributor

@seemk seemk left a comment

Choose a reason for hiding this comment

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

Sure, one idea would be to separate owners by directories. 🙂

@reyang
Copy link
Member

reyang commented Mar 30, 2021

Sure, one idea would be to separate owners by directories. 🙂

I like this idea. OpenTelemetry .NET Contrib is doing this and it seems to be working well.

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM.

@lalitb
Copy link
Member Author

lalitb commented Mar 30, 2021

Sure, one idea would be to separate owners by directories. 🙂

Good point. This was how my initial commit to this PR looked like ( 20f5559 )

We can discuss and revert back to something similar if all agree.

@lalitb
Copy link
Member Author

lalitb commented Mar 31, 2021

Sure, one idea would be to separate owners by directories. 🙂

Good point. This was how my initial commit to this PR looked like ( 20f5559 )

We can discuss and revert back to something similar if all agree.

Have switched to directory level owners now.

@seemk
Copy link
Contributor

seemk commented Mar 31, 2021

Sure, one idea would be to separate owners by directories. slightly_smiling_face

Good point. This was how my initial commit to this PR looked like ( 20f5559 )
We can discuss and revert back to something similar if all agree.

Have switched to directory level owners now.

Thanks! Btw safe to ignore the failing test. Reworked the tests in #10 a bit (also so it won't trigger on unrelated changes anymore)

Copy link
Member

@TomRoSystems TomRoSystems left a comment

Choose a reason for hiding this comment

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

Remove * at the end of path

Comment on lines 10 to 11
instrumentation/httpd/* @TomRoSystems @open-telemetry/cpp-contrib-approvers
instrumentation/nginx/* @seemk @open-telemetry/cpp-contrib-approvers
Copy link
Member

Choose a reason for hiding this comment

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

The problem with this is that * at the end of path will not match subdirectories.

Also I was thinking about some extra rule for CODEOWNERS file itself? I was not aware of this PR and created #13 but please close it was this one is merged.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. Have removed the *. I am not sure whether we want to have separate maintainers for contrib repo, or let approvers own this responsibility jointly here. We can discuss.

@lalitb lalitb merged commit 5d3c853 into open-telemetry:main Apr 1, 2021
@marcalff marcalff added the general General issue, not specific to a particular contrib label May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
general General issue, not specific to a particular contrib
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants