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

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

Merged
merged 2 commits into from
Aug 31, 2023
Merged

Conversation

yeikel
Copy link
Contributor

@yeikel yeikel commented Aug 14, 2023

The latest version of Node 18 installs npm 9.6.7 but we need more information before we can proceed. See npm/cli#6742

In the meantime, I believe that it adds value to upgrade to 9.6.5 as we're behind

Closes #7659

@yeikel yeikel requested a review from a team as a code owner August 14, 2023 02:25
@yeikel yeikel changed the title build(deps): bump npm to 9.6.7 build(deps): bump npm from 9.5.1 to 9.6.7 Aug 14, 2023
@yeikel yeikel force-pushed the 9.6.7 branch 2 times, most recently from 5881628 to 1b2a020 Compare August 15, 2023 23:07
@yeikel yeikel marked this pull request as draft August 15, 2023 23:21
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

R

@yeikel yeikel force-pushed the 9.6.7 branch 2 times, most recently from b60f4fc to ab10b72 Compare August 28, 2023 01:56
@yeikel yeikel changed the title build(deps): bump npm from 9.5.1 to 9.6.7 build(deps): bump npm from 9.5.1 to 9.6.6 Aug 28, 2023
@yeikel yeikel changed the title build(deps): bump npm from 9.5.1 to 9.6.6 build(deps): bump npm from 9.5.1 to 9.6.5 Aug 28, 2023
@yeikel yeikel force-pushed the 9.6.7 branch 2 times, most recently from 0293998 to 137b33a Compare August 28, 2023 02:01
@@ -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

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.

@yeikel yeikel marked this pull request as ready for review August 28, 2023 02:14
@yeikel
Copy link
Contributor Author

yeikel commented Aug 28, 2023

The current failing spec seems flaky because I cannot reproduce it locally

This error also seems like a different bug but I do not know how to reproduce it :

Failure/Error:
@project_npm_response.body.force_encoding("UTF-8").encode.
include?(project_description)

 FrozenError:
   can't modify frozen String: ""

I am also seeing the same failure in #7908

@yeikel
Copy link
Contributor Author

yeikel commented Aug 28, 2023

The current failing spec seems flaky because I cannot reproduce it locally

This error also seems like a different bug but I do not know how to reproduce it :

Failure/Error:
@project_npm_response.body.force_encoding("UTF-8").encode.
include?(project_description)

 FrozenError:
   can't modify frozen String: ""

I am also seeing the same failure in #7908

It seems that 4999e9b05cc62e5ad5c6331db87a3b40a17f2946 fixed it. Should I send a separate pull request for it?

@jeffwidman
Copy link
Member

The current failing spec seems flaky because I cannot reproduce it locally
This error also seems like a different bug but I do not know how to reproduce it :

Failure/Error:
@project_npm_response.body.force_encoding("UTF-8").encode.
include?(project_description)

 FrozenError:
   can't modify frozen String: ""

I am also seeing the same failure in #7908

It seems that 4999e9b fixed it. Should I send a separate pull request for it?

Yes please, seeing as this likely exists on main, can you please break out a separate PR? That way if we revert this npm bump we won't lose it...

@yeikel yeikel force-pushed the 9.6.7 branch 2 times, most recently from 12d0106 to 68bbf86 Compare August 30, 2023 01:22
@yeikel
Copy link
Contributor Author

yeikel commented Aug 30, 2023

The current failing spec seems flaky because I cannot reproduce it locally
This error also seems like a different bug but I do not know how to reproduce it :

Failure/Error:
@project_npm_response.body.force_encoding("UTF-8").encode.
include?(project_description)

 FrozenError:
   can't modify frozen String: ""

I am also seeing the same failure in #7908

It seems that 4999e9b fixed it. Should I send a separate pull request for it?

Yes please, seeing as this likely exists on main, can you please break out a separate PR? That way if we revert this npm bump we won't lose it...

Thanks, I dropped the commit from this branch and submitted this specific change with #7926

This branch is expected to continue failing until that is merged

@yeikel yeikel requested a review from jeffwidman August 30, 2023 01:40
@yeikel
Copy link
Contributor Author

yeikel commented Aug 30, 2023

@jeffwidman After #7926 now this, #7899 and #7908 are ready to be reviewed

I don't have any rush though :)

@jeffwidman jeffwidman merged commit 9fbb0c9 into dependabot:main Aug 31, 2023
79 checks passed
@yeikel yeikel deleted the 9.6.7 branch August 31, 2023 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants