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

Change Node.js CODEOWNERS to the Node.js guild #3795

Merged

Conversation

watson
Copy link
Contributor

@watson watson commented Jan 10, 2025

Motivation

I think the old CODEOWNERS is a legacy setting and that it actually should be the guild(?)

Changes

Change the Node.js CODEOWNERS from @DataDog/apm-js and @DataDog/asm-js to @DataDog/dd-trace-js

Workflow

  1. ⚠️ Create your PR as draft ⚠️
  2. Work on you PR until the CI passes (if something not related to your task is failing, you can ignore it)
  3. Mark it as ready for review
    • Test logic is modified? -> Get a review from RFC owner. We're working on refining the codeowners file quickly.
    • Framework is modified, or non obvious usage of it -> get a review from R&P team

🚀 Once your PR is reviewed, you can merge it!

🛟 #apm-shared-testing 🛟

Reviewer checklist

  • If PR title starts with [<language>], double-check that only <language> is impacted by the change
  • No system-tests internal is modified. Otherwise, I have the approval from R&P team
  • CI is green, or failing jobs are not related to this change (and you are 100% sure about this statement)
  • A docker base image is modified?
    • the relevant build-XXX-image label is present
  • A scenario is added (or removed)?

@watson watson requested a review from a team as a code owner January 10, 2025 05:34
Copy link
Contributor Author

watson commented Jan 10, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

@watson watson requested review from a team January 10, 2025 05:35
@simon-id
Copy link
Member

simon-id commented Jan 10, 2025

I think you're right that those two codeowners are obsolete now that more and more teams are using this file, but not sure if dd-trace-js should be the right one. There are a lot of people in that team that would receive emails for every PR and that will probably not care ?
Is there another team that would be more relevant instead ? or should we just keep apm-js, asm-js and add yours ?

@watson
Copy link
Contributor Author

watson commented Jan 10, 2025

What members in dd-trace-js shouldn't also be involved in Node.js related system tests in one way or another? Similar to how all members of dd-trace-js should be involved in our regular tests that exists inside the dd-trace-js repo.

@simon-id
Copy link
Member

Ok, good point. I just try to not add too much noise to people's info streams

@watson watson merged commit db04b2a into main Jan 13, 2025
280 of 281 checks passed
@watson watson deleted the watson/01-10-change_node.js_codeowners_to_the_node.js_guild branch January 13, 2025 08:58
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.

2 participants