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

build(deps): bump npm from 9.5.1 to 9.6.5 #7811

Merged
merged 2 commits into from
Aug 31, 2023
Merged

build(deps): bump npm from 9.5.1 to 9.6.5 #7811

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e5836a1
build(deps): bump npm from 9.5.1 to 9.6.5
yeikel Aug 14, 2023
837da52
Merge branch 'main' into 9.6.7
yeikel Aug 31, 2023
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
3 changes: 2 additions & 1 deletion npm_and_yarn/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ ARG NODEJS_VERSION=18.x

# Check for updates at https://github.com/npm/cli/releases
# This version should be compatible with the Node.js version declared above. See https://nodejs.org/en/download/releases as well
ARG NPM_VERSION=9.5.1
# TODO: Upgrade to 9.6.7 depending on the outcome of https://github.com/npm/cli/issues/6742
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure if this TODO is appropriate or if I should create a separate dependabot-core issue instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@deivid-rodriguez @jeffwidman What do you recommend?

Copy link
Member

@jeffwidman jeffwidman Aug 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'm cool with either, especially as the TODO has an obvious location right next to the version string and right now we've chosen to focus on investing heavily into paying down tech debt on a few specific ecosystems rather than closely tracking issues.

Personally, I often tend to do both adding a TODO in code and then also adding an issue pointing at the TODO, but then I'm an overkill kind of guy for this sort of thing. 😁

What you've got is perfectly fine IMO. Also, the ones I really like to add TODOs for are the ones where we'll lose the context/realization that something needs to happen if a comment/issue isn't filed. The ones where it's like "down the road we'll realize the problem" are NBD IMO, and being behind on versions is one of them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I'll leave it as is for now as that makes sense

I do agree that TODOs can become a problem if they don't have enough context as I see that often and most of the time the only context are commits from many years ago that makes it very hard to decide if that's still needed

Copy link
Member

Choose a reason for hiding this comment

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

Some clarity on the TODO:

ARG NPM_VERSION=9.6.5

# Install Node and npm
RUN curl -sL https://deb.nodesource.com/setup_$NODEJS_VERSION | bash - \
Expand Down
Loading