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

Update CODEOWNERS file #3143

Merged
merged 6 commits into from
Sep 25, 2023
Merged

Update CODEOWNERS file #3143

merged 6 commits into from
Sep 25, 2023

Conversation

GustavoCaso
Copy link
Member

@GustavoCaso GustavoCaso commented Sep 19, 2023

What does this PR do?

Update the CODEOWNER files to assign teams better when specific files are modified.

We still have a catch-all * @DataDog/ruby-guild, meaning every team would get a ping for files not specified on the other rules.

Of course we can tweak and improve those rules if we want

Motivation:

Additional Notes:

How to test the change?

For Datadog employees:

  • If this PR touches code that signs or publishes builds or packages, or handles
    credentials of any kind, I've requested a review from @DataDog/security-design-and-guidance.
  • This PR doesn't touch any of that.

Unsure? Have a question? Request a review!

@GustavoCaso GustavoCaso requested a review from a team September 19, 2023 07:56
@github-actions github-actions bot added the dev/github Github repository maintenance and automation label Sep 19, 2023
@codecov-commenter
Copy link

codecov-commenter commented Sep 19, 2023

Codecov Report

Merging #3143 (f893a1c) into master (cb5fe8a) will not change coverage.
Report is 20 commits behind head on master.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3143   +/-   ##
=======================================
  Coverage   98.16%   98.16%           
=======================================
  Files        1283     1283           
  Lines       73915    73915           
  Branches     3425     3425           
=======================================
  Hits        72558    72558           
  Misses       1357     1357           

see 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

lib/datadog/profiling/* @DataDog/profiling-rb
lib/datadog/profiling.rb @DataDog/profiling-rb
ext/* @DataDog/profiling-rb
Copy link
Contributor

Choose a reason for hiding this comment

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

This question is probably outside the scope of this specific PR, but should we rename ext/ since it appears to be a profiling only folder?

Copy link
Member

Choose a reason for hiding this comment

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

We can't as ext/ is a standard for Ruby gems with native extensions: https://guides.rubygems.org/gems-with-extensions/

Copy link
Contributor

Choose a reason for hiding this comment

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

My rationale here is:

  • ext being a generic folder for extensions, it should be owned by the guild.
  • ext/ddtrace_profiling_loader and ext/ddtrace_profiling_native_extension should be owned by profiling-rb

Copy link
Contributor

@lloeki lloeki Sep 20, 2023

Choose a reason for hiding this comment

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

That said, currently profiling is the sole user of libdatadog, but that may/should change, therefore I would like to encourage ownership to be split:

  • extension code pertaining to profiling should live in its own profiling-owned subdirectories
  • extension code pertaining to libdatadog in general should live in guild-owned subdirectories

e.g:

  • ddtrace_profiling_loader has no relationship to profiling at all, merely libdatadog. I suggest we rename it to libdatadog_loader and share ownership.
  • ddtrace_profiling_native_extension may or may not contain code that pertains only to libdatadog in general. If so, I'd suggest splitting it away in some way and share ownership of the common bits.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can't as ext/ is a standard for Ruby gems with native extensions

Note that is is mostly conventional, The directory being ext is not actually technically required, but I would definitely refrain from changing it to anything else!

Copy link
Contributor

Choose a reason for hiding this comment

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

That said, currently profiling is the sole user of libdatadog, but that may/should change

That's a good point. I think that may change in the not too distant future.

.github/CODEOWNERS Outdated Show resolved Hide resolved
lib/datadog/opentracer/* @DataDog/tracing-ruby
lib/datadog/opentracer.rb @DataDog/tracing-ruby
lib-injection/* @DataDog/tracing-ruby
Copy link
Contributor

Choose a reason for hiding this comment

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

I am questioning this a bit: while initially developed by tracing automatic injection sounds like a feature that applies to everyone, not just tracing.

/docs/GettingStarted.md @DataDog/documentation

# Library
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this file is missing sig ownership

Copy link
Contributor

Choose a reason for hiding this comment

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

Added!

@GustavoCaso GustavoCaso merged commit ba9fc95 into master Sep 25, 2023
176 checks passed
@GustavoCaso GustavoCaso deleted the update-codeowners branch September 25, 2023 09:28
@github-actions github-actions bot added this to the 1.15.0 milestone Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev/github Github repository maintenance and automation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants