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

Jump to bottom

deps: fix npm files from upgrade to 4.1.2 #11085

Closed
wants to merge 1 commit into from
Closed

deps: fix npm files from upgrade to 4.1.2 #11085

wants to merge 1 commit into from

Conversation

joaocgreis
Copy link
Member

#11020 landed adding some files that look like merge temporary files, that are not handled well by Git on Windows. This PR just removes those files.

Is there any objection to fasttracking this? Windows CI is failing all runs for master: https://ci.nodejs.org/view/All/job/node-compile-windows/

cc @nodejs/npm @zkat @Fishrock123

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

Deps

@joaocgreis joaocgreis added the npm Issues and PRs related to the npm client dependency or the npm registry. label Jan 31, 2017
@nodejs-github-bot nodejs-github-bot added the npm Issues and PRs related to the npm client dependency or the npm registry. label Jan 31, 2017
@mscdex
Copy link
Contributor

mscdex commented Jan 31, 2017 •
edited
Loading

Yeah I'm surprised there wasn't a CI run for that PR...

jasnell
jasnell approved these changes Jan 31, 2017
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

:-( .... +1 for fast track. The original PR should not have landed in that state

@joaocgreis
Copy link
Member Author

CI: https://ci.nodejs.org/view/All/job/node-test-pull-request/6128/

@gibfahn
Copy link
Member

gibfahn commented Jan 31, 2017 •
edited
Loading

+1 for fast-track

I think the reason CI wasn't run is that there isn't much that tests deps/npm/ in the native CI (except for test/parallel/test-npm-install.js). However, I think in general that it's always worth running CI, even if we think that it won't catch anything.

If I can get #7867 in then we would be able to run the npm tests separately.

@Trott Trott mentioned this pull request Jan 31, 2017
Closed
2 tasks
Trott
Trott approved these changes Jan 31, 2017
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM. +1 to landing now. (All but 3 CI hosts have finished. Most importantly, Windows is passing.)

joaocgreis added a commit that referenced this pull request Jan 31, 2017
@joaocgreis
deps: fix npm files from upgrade to 4.1.2
35e749b 
PR-URL: #11085
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@joaocgreis
Copy link
Member Author

Landed in 35e749b

@joaocgreis joaocgreis closed this Jan 31, 2017
@sam-github sam-github mentioned this pull request Jan 31, 2017
Closed
3 tasks
@zkat
Copy link
Contributor

zkat commented Jan 31, 2017

Gonna toss my belated LGTM in there and thanks for fixing these :)

@Fishrock123
Copy link
Contributor

😬 Thanks for merging!

evanlucas pushed a commit that referenced this pull request Jan 31, 2017
@joaocgreis @evanlucas
deps: fix npm files from upgrade to 4.1.2
84a9c15 
PR-URL: #11085
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@Fishrock123 Fishrock123 mentioned this pull request Apr 24, 2017
Closed
Fishrock123 added a commit that referenced this pull request Apr 26, 2017
@Fishrock123
deps: remove **/node_modules/form-data/README.md
e8f3163 
Having multiple files with the same name but different casings causes
problems on lots of OS-s.

Refs: #12624
Refs: #11085
Refs: #11020

PR-URL: #12643
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
evanlucas pushed a commit that referenced this pull request May 1, 2017
@Fishrock123 @evanlucas
deps: remove **/node_modules/form-data/README.md
41e052a 
Having multiple files with the same name but different casings causes
problems on lots of OS-s.

Refs: #12624
Refs: #11085
Refs: #11020

PR-URL: #12643
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
evanlucas pushed a commit that referenced this pull request May 2, 2017
@Fishrock123 @evanlucas
deps: remove **/node_modules/form-data/README.md
f8ae23e 
Having multiple files with the same name but different casings causes
problems on lots of OS-s.

Refs: #12624
Refs: #11085
Refs: #11020

PR-URL: #12643
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
evanlucas pushed a commit that referenced this pull request May 2, 2017
@Fishrock123 @evanlucas
deps: remove **/node_modules/form-data/README.md
d154aaf 
Having multiple files with the same name but different casings causes
problems on lots of OS-s.

Refs: #12624
Refs: #11085
Refs: #11020

PR-URL: #12643
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@targos targos targos approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
npm Issues and PRs related to the npm client dependency or the npm registry.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@joaocgreis @mscdex @gibfahn @zkat @Fishrock123 @jasnell @Trott @targos @nodejs-github-bot