GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — meta: update comment in `CODEOWNERS` to better reflect current policy by aduh95 · Pull Request #45944 · nodejs/node

Conversation

@aduh95

@aduh95 aduh95 commented Dec 22, 2022

Copy link
Copy Markdown
Contributor

We do not enforce signing off by any code owners when landing PRs.

We may want to take the opportunity to refine our policy regarding CODEOWNERS and what are the criteria to consider a PR ready to land. Currently, here are the criteria listed in the Collaborator Guide:

node/doc/contributing/collaborator-guide.md

Lines 139 to 147 in 6831e2f

### Consensus seeking
A pull request can land if it has the needed [approvals](#code-reviews),
[CI](#testing-and-ci), [wait time](#waiting-for-approvals) and no
[outstanding objections](#objections). [Breaking changes](#breaking-changes)
must receive [TSC review](#involving-the-tsc) in addition to other
requirements. If a pull request meets all requirements except the
[wait time](#waiting-for-approvals), please add the
[`author ready`](#author-ready-pull-requests) label.

node/doc/contributing/collaborator-guide.md

Lines 122 to 129 in 6831e2f

At least two collaborators must approve a pull request before the pull request
lands. One collaborator approval is enough if the pull request has been open
for more than seven days.
Approving a pull request indicates that the collaborator accepts responsibility
for the change.
Approval must be from collaborators who are not authors of the change.

/cc @GeoffreyBooth @nodejs/tsc

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@aduh95
meta: update comment in CODEOWNERS to better reflect current policy
ee6e881 
We do not enforce signing off by any code owners when landing PRs.
@nodejs-github-bot

nodejs-github-bot commented Dec 22, 2022

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/tsc
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot nodejs-github-bot added the meta Issues and PRs related to the general management of the project. label Dec 22, 2022
@GeoffreyBooth

GeoffreyBooth commented Dec 22, 2022

Copy link
Copy Markdown
Member

Rather than restate the policy from the collaborator guide, perhaps this comment should just link to it?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
lpinca
lpinca approved these changes Dec 22, 2022
View reviewed changes
RaisinTen
RaisinTen approved these changes Dec 23, 2022
View reviewed changes

@RaisinTen RaisinTen left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM. I agree with @GeoffreyBooth, doc/contributing/collaborator-guide.md is a much better place for this info because all collaborators are familiar with it. I don't think many collaborators will look at comments in a GitHub specific file for our policies.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
joyeecheung
joyeecheung approved these changes Dec 26, 2022
View reviewed changes
GeoffreyBooth
GeoffreyBooth reviewed Dec 26, 2022
View reviewed changes
Comment thread .github/CODEOWNERS Resolved

Uh oh!

There was an error while loading. Please reload this page.
@GeoffreyBooth @aduh95
Update .github/CODEOWNERS
210d7a6 
Co-authored-by: Antoine du Hamel <[email protected]>
GeoffreyBooth
GeoffreyBooth approved these changes Dec 28, 2022
View reviewed changes
@GeoffreyBooth GeoffreyBooth added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Dec 28, 2022
@aduh95
Remove the ordered list
604c03b
@aduh95

aduh95 commented Dec 28, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

I've added a few additional changes which after giving it a few thoughts, I think should belong in this PR:

  • I've removed the ordered list, AFAICT there was no particular order in the list.
  • I've removed the point that says "Each codeowner team should contain at least one @nodejs/tsc member" because I believe we do not enforce this either.

PTAL if you already approved the PR, and consider re-approving if you agree with that new change.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95 aduh95 removed the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Dec 28, 2022
GeoffreyBooth
GeoffreyBooth approved these changes Dec 29, 2022
View reviewed changes
Trott
Trott approved these changes Jan 1, 2023
View reviewed changes
@aduh95 aduh95 added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Jan 1, 2023
legendecas
legendecas approved these changes Jan 3, 2023
View reviewed changes
mhdawson
mhdawson approved these changes Jan 3, 2023
View reviewed changes

@mhdawson mhdawson left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
BethGriggs
BethGriggs approved these changes Jan 4, 2023
View reviewed changes
danielleadams
danielleadams approved these changes Jan 4, 2023
View reviewed changes
@aduh95 aduh95 added commit-queue PRs queued for automated landing through the Commit Queue. commit-queue-squash PRs the Commit Queue should land as one squashed commit. labels Jan 6, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Jan 6, 2023
@nodejs-github-bot
nodejs-github-bot merged commit 6f50acd into nodejs:main Jan 6, 2023
@nodejs-github-bot

nodejs-github-bot commented Jan 6, 2023

Copy link
Copy Markdown
Collaborator

Landed in 6f50acd
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95
aduh95 deleted the update-codeowners-comment branch January 6, 2023 09:57
RafaelGSS pushed a commit to RafaelGSS/node that referenced this pull request Jan 17, 2023
@aduh95 @RafaelGSS
meta: update comment in CODEOWNERS to better reflect current policy
20d76d8 
We do not enforce signing off by any code owners when landing PRs.

PR-URL: nodejs#45944
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Danielle Adams <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Jan 20, 2023
@aduh95 @RafaelGSS
meta: update comment in CODEOWNERS to better reflect current policy
1b557bb 
We do not enforce signing off by any code owners when landing PRs.

PR-URL: #45944
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Danielle Adams <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Jan 20, 2023
v19.5.0 proposal #46286
Merged
juanarbol pushed a commit that referenced this pull request Jan 26, 2023
@aduh95 @juanarbol
meta: update comment in CODEOWNERS to better reflect current policy
08a9bf8 
We do not enforce signing off by any code owners when landing PRs.

PR-URL: #45944
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Danielle Adams <[email protected]>
@juanarbol juanarbol mentioned this pull request Jan 28, 2023
V18.14.0 proposal #46396
Merged
juanarbol pushed a commit that referenced this pull request Jan 31, 2023
@aduh95 @juanarbol
meta: update comment in CODEOWNERS to better reflect current policy
d8ce990 
We do not enforce signing off by any code owners when landing PRs.

PR-URL: #45944
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Danielle Adams <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@GeoffreyBooth GeoffreyBooth GeoffreyBooth approved these changes
@Trott Trott Trott approved these changes
@lpinca lpinca lpinca approved these changes
@joyeecheung joyeecheung joyeecheung approved these changes
@BethGriggs BethGriggs BethGriggs approved these changes
@legendecas legendecas legendecas approved these changes
@RaisinTen RaisinTen RaisinTen approved these changes
+2 more reviewers
@danielleadams danielleadams danielleadams approved these changes
@mhdawson mhdawson mhdawson approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. commit-queue-squash PRs the Commit Queue should land as one squashed commit. meta Issues and PRs related to the general management of the project.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

11 participants

@aduh95 @nodejs-github-bot @GeoffreyBooth @Trott @lpinca @joyeecheung @danielleadams @BethGriggs @legendecas @mhdawson @RaisinTen
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.