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.

Sign up for GitHub

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

doc: add additional guidance for PRs to deps #53499

Closed
wants to merge 4 commits into from
Closed

doc: add additional guidance for PRs to deps #53499

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ec3ed97
doc: add additional guidance for PRs to deps
mhdawson Jun 18, 2024
be913d0
address comments
mhdawson Jun 19, 2024
748e8ca
Update maintaining-dependencies.md
mhdawson Jun 19, 2024
12b9007
squash: address comments
mhdawson Jun 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions doc/contributing/collaborator-guide.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,11 @@ for the change.

Approval must be from collaborators who are not authors of the change.

Ideally pull requests for dependencies should be generated by automation.
Pay special attention to pull requests for dependencies which have not
been automatically generated and follow the guidance in
[Maintaining Dependencies](https://github.com/nodejs/node/blob/main/doc/contributing/maintaining/maintaining-dependencies.md#updating-dependencies).

In some cases, it might be necessary to summon a GitHub team to a pull request
for review by @-mention.
See [Who to CC in the issue tracker](#who-to-cc-in-the-issue-tracker).
Expand Down
11 changes: 11 additions & 0 deletions doc/contributing/maintaining/maintaining-dependencies.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,17 @@ the corresponding script in `tools/update-deps`.
[npm-cli-bot](https://github.com/npm/cli/blob/latest/.github/workflows/create-node-pr.yml)
takes care of npm update, it is maintained by the npm team.

PRs for manual dependency updates should only be accepted if
the update cannot be generated by the automated tooling,
the reason is clearly documented and either the PR is
reviewed in detail or it is from an existing collaborator.
mhdawson marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +147 to +150
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a note about only accepting changes that have landed upstream, and "the TSC may grant exception on a case-by-case basis"? I think it's already more or less the policy we're currently organically following, so IMO it'd make sense explicitly state it out.

Copy link
Member Author

Choose a reason for hiding this comment

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

@aduh95 that is a good idea.

Copy link
Member Author

@mhdawson mhdawson Jun 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@aduh95 added some wording along those lines.


In general updates to dependencies should only be accepted
if they have already landed in the upstream. The TSC may
grant an exception on a case-by-case basis. This avoids
the project having to float patches for a long time and
ensures that tooling can generate updates automatically.

## Dependency list

### acorn
Expand Down