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

doc: remove version from maintaining-dependencies.md #51195

Merged
merged 5 commits into from
Dec 19, 2023

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Dec 17, 2023

Those create unreasonable maintenance burden as updating one dependency will conflict with another one. It's unclear we get much value out from duplicating this information in this document, so this PR removes it.

Those create unreasonable maintenance burden as updating one dependency
will conflict with another one. It's unclear we get much value out from
duplicating this information in this document, so this commit removes it
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/security-wg

@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Dec 17, 2023
@marco-ippolito
Copy link
Member

marco-ippolito commented Dec 17, 2023

I have mixed feelings, the maintainance burden is annoying but not having the dependency version tracked in a readable way sounds bad too.
We are working on SBOM that will fix this nodejs/security-wg#1115

@aduh95
Copy link
Contributor Author

aduh95 commented Dec 17, 2023

I think the point of having a workflow that automatically updates our deps is to stay up-to-date, and not burn out our volunteers' time with busy work, and if you look at the number of open PRs for dependency updates, you can see we're falling behind – because no one wants to do the work of fixing conflicts, I myself regret spending the time I spent on this when I see that as soon as one PR has landed, it has undone the work I just did by creating new conflicts.
Also, when Undici 6.x upgrade PR will have landed as a semver-major, it's going to create conflicts when backporting to LTS lines, adding more work for the release team.

but not having the dependency version tracked in a readable way sounds bad too.

OK but not landing the upgrade PRs is worse I think, and that's the current situation.

The current situation has to change if you want me to keep interacting with those upgrade PRs, it's simply not worth my time to try to fix an endless flow of git conflicts – and more importantly, it's not worth releasers' time either.

If you want a readable way to get the information, hopefully using git is enough? For example, if you go to https://github.com/nodejs/node/tree/main/deps, GitHub web UI will list the last commit and that's often the last update with the version number:
screenshot of GitHub web UI
Another solution would be to keep the version in maintaining-dependencies.md, but in a place where it couldn't conflict.

Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

I agree 100%

Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

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

I already spent a long time (accumulated) fixing conflicts in LTS

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Dec 18, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 19, 2023
@nodejs-github-bot nodejs-github-bot merged commit 898149f into nodejs:main Dec 19, 2023
26 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 898149f

@aduh95 aduh95 deleted the no-version-in-maintaining-md branch December 19, 2023 10:04
ionicmc-js pushed a commit to ionicmc-js/node that referenced this pull request Dec 19, 2023
Those create unreasonable maintenance burden as updating one dependency
will conflict with another one. It's unclear we get much value out from
duplicating this information in this document, so this commit removes it

PR-URL: nodejs#51195
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM. I think this made sense when we were doing it manually, but now that we have automation doing the updates the conflicts are not worty the overhead.

RafaelGSS pushed a commit that referenced this pull request Jan 2, 2024
Those create unreasonable maintenance burden as updating one dependency
will conflict with another one. It's unclear we get much value out from
duplicating this information in this document, so this commit removes it

PR-URL: #51195
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Jan 2, 2024
richardlau pushed a commit that referenced this pull request Mar 25, 2024
Those create unreasonable maintenance burden as updating one dependency
will conflict with another one. It's unclear we get much value out from
duplicating this information in this document, so this commit removes it

PR-URL: #51195
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
@richardlau richardlau mentioned this pull request Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants