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

Fix issue #573 - private git ssh urls #1816

Closed
wants to merge 53 commits into from

Conversation

f-sign
Copy link
Contributor

@f-sign f-sign commented Nov 14, 2016

Summary

Yarn didn't support certain formats of git urls, especially the ones with colon on it.

Test plan

Added tests to reproduce the problem and these tests all pass.

Copy link
Member

@wyze wyze left a comment

Choose a reason for hiding this comment

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

There are quite a few commented out console.log statements in here that need to be removed.

@BryanCrotaz
Copy link

@wyze the CI seems to have failed due to missing public keys - can you re-run?

@wyze
Copy link
Member

wyze commented Nov 17, 2016

@BryanCrotaz, they are rerunning now.

@toefraz
Copy link

toefraz commented Nov 18, 2016

@wyze it seems like CI failed again due to key errors installing from GitHub

@chrisirhc
Copy link
Contributor

Any chance someone with the permissions can re-run the tests?

@wyze
Copy link
Member

wyze commented Nov 22, 2016

Kicked off a rebuild on Travis.

@BryanCrotaz
Copy link

@wyze Failed again with Permission denied (publickey)

@wyze
Copy link
Member

wyze commented Nov 22, 2016

Other builds are passing. This PR touches the hosted-git-resolver which could cause this test to fail. @f-sign, could you please check into the test failure?

@BryanCrotaz
Copy link

@wyze it's not getting as far as running the tests:

(node:8486) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): Error: Error: Command failed.
Exit code: 128
Command: git
Arguments: clone ssh://[email protected]/substack/node-mkdirp.git /tmp/yarn-install-github-0.3094830624742697/.yarn-cache/.tmp/f87fb05e5feff74616d48fa358e1d0ce
Directory: /home/travis/build/yarnpkg/yarn
Output:
Cloning into '/tmp/yarn-install-github-0.3094830624742697/.yarn-cache/.tmp/f87fb05e5feff74616d48fa358e1d0ce'...
Warning: Permanently added the RSA host key for IP address '192.30.253.112' to the list of known hosts.
Permission denied (publickey).
fatal: Could not read from remote repository.
Please make sure you have the correct access rights
and the repository exists.
    at MessageError (/home/travis/build/yarnpkg/yarn/src/errors.js:5:5)
    at SpawnError (/home/travis/build/yarnpkg/yarn/src/errors.js:14:1)
    at ChildProcess.proc.on.code (/home/travis/build/yarnpkg/yarn/src/util/child.js:86:17)
    at emitTwo (events.js:106:13)
    at ChildProcess.emit (events.js:191:7)
    at maybeClose (internal/child_process.js:877:16)
    at Socket.<anonymous> (internal/child_process.js:334:11)
    at emitOne (events.js:96:13)
    at Socket.emit (events.js:188:7)
    at Pipe._handle.close [as _onclose] (net.js:498:12) 

The PR doesn't touch the package.json so I can't see how this PR would break it

@BryanCrotaz
Copy link

@wyze DOH!!!! I'm an idiot. Ignore that last message. I'll ask Flavio to take another look.

@toefraz
Copy link

toefraz commented Nov 30, 2016

Any updates on why the CI tests are failing?

@wyze
Copy link
Member

wyze commented Nov 30, 2016

@toefraz, waiting to hear back from @f-sign to take a look. Will also need a rebase.

@BryanCrotaz
Copy link

@f-sign and I have been looking at it - can't work out how taking some comments out has caused the build to fail

f-sign and others added 6 commits December 6, 2016 14:55
…itten (yarnpkg#1970)

* Fix for yarnpkg#1214

Scoped dependencies are nested one folder deeper than traditional
dependencies, the possiblyExtraneous needed updating to work with the
actual dependencies, not the entire scoped folder.

* Fix typo/variable renaming
* Use isRootUser function instead USER environment variable
 * Fix yarnpkg#2123
* Add failing tests for bin path normalisation from scoped package name

* Add fix for bin normalisation for scoped packages
@cascornelissen
Copy link

@BryanCrotaz, any updates on this? What's the plan?

@f-sign
Copy link
Contributor Author

f-sign commented Dec 8, 2016

Sorry for the delay... Some deadlines here.

We've run the tests locally against latest master and our PR branch. We get the same 3 failures on both:

MASTER eb66b9c

Summary of all failing tests
 FAIL  __tests__\commands\cache.js (120.793s)
  ● write cache-folder config into .yarnrc file

    Timeout - Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL.

      at Timeout._onTimeout (node_modules\jest-jasmine2\vendor\jasmine-2.4.1.js:1912:23)
      at ontimeout (timers.js:365:14)
      at tryOnTimeout (timers.js:237:5)
      at Timer.listOnTimeout (timers.js:207:5)

  ● cache-folder flag has higher priorities than .yarnrc file

    Timeout - Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL.

      at Timeout._onTimeout (node_modules\jest-jasmine2\vendor\jasmine-2.4.1.js:1912:23)
      at ontimeout (timers.js:365:14)
      at tryOnTimeout (timers.js:237:5)
      at Timer.listOnTimeout (timers.js:207:5)

 FAIL  __tests__\commands\add.js (154.473s)
  ● add with new dependency should be deterministic 3

    Timeout - Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL.

      at Timeout._onTimeout (node_modules\jest-jasmine2\vendor\jasmine-2.4.1.js:1912:23)
      at ontimeout (timers.js:365:14)
      at tryOnTimeout (timers.js:237:5)
      at Timer.listOnTimeout (timers.js:207:5)


Test Suites: 2 failed, 1 skipped, 38 passed, 40 of 41 total
Tests:       3 failed, 19 skipped, 292 passed, 314 total
Snapshots:   50 passed, 50 total
Time:        177.557s
Ran all test suites.

Mine 9780017

Summary of all failing tests
 FAIL  __tests__\commands\cache.js (120.795s)
  ● write cache-folder config into .yarnrc file

    Timeout - Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL.

      at Timeout._onTimeout (node_modules\jest-jasmine2\vendor\jasmine-2.4.1.js:1912:23)
      at ontimeout (timers.js:365:14)
      at tryOnTimeout (timers.js:237:5)
      at Timer.listOnTimeout (timers.js:207:5)

  ● cache-folder flag has higher priorities than .yarnrc file

    Timeout - Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL.

      at Timeout._onTimeout (node_modules\jest-jasmine2\vendor\jasmine-2.4.1.js:1912:23)
      at ontimeout (timers.js:365:14)
      at tryOnTimeout (timers.js:237:5)
      at Timer.listOnTimeout (timers.js:207:5)

 FAIL  __tests__\commands\add.js (152.401s)
  ● add with new dependency should be deterministic 3

    Timeout - Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL.

      at Timeout._onTimeout (node_modules\jest-jasmine2\vendor\jasmine-2.4.1.js:1912:23)
      at ontimeout (timers.js:365:14)
      at tryOnTimeout (timers.js:237:5)
      at Timer.listOnTimeout (timers.js:207:5)


Test Suites: 2 failed, 1 skipped, 38 passed, 40 of 41 total
Tests:       3 failed, 19 skipped, 296 passed, 318 total
Snapshots:   50 passed, 50 total
Time:        175.15s
Ran all test suites.

The first time I submitted the pull request it passed all tests and I was asked to remove some commented out console.log(). After removing then, it is not passing one of the tests. I am running the tests on Windows.
Any advice?

torifat and others added 2 commits December 8, 2016 16:13
…npkg#2063)

- Only look for mandetory files (`package.json`, `readme`, `license`, etc) in root
- Stop `fs.walk` inside ignored dirs (`node_modules`, `.git`, `.svn`, etc), makes `pack` way faster
iredchuk and others added 2 commits December 19, 2016 03:42
* Add support for disabling progress bar

* after config is initialized, check global yarn config for disableProgress

* Add utility function to disable progress on base-reporter

* Add base reporter disableProgress test
@epmatsw
Copy link

epmatsw commented Dec 19, 2016

On macOS 10.12.2, node 7.2.0, and npm 3.10.9, I had one run that had a single failure (something about Cannot find module '../lib-legacy/constants'). After removing node_modules and re-running yarn, I ran the tests again and got that failure plus the async failures mentioned above for install with arg that has install scripts and install should hoist nested bin scripts.

The original test asserts that window cmd line formatting is properly applied or not applied, for only the target platform the tests are executed on - that will have to be good enough.

It is not possible to test both branches from one platform because the path module employed in the test alters its export based on process.platform in Node bootstrap, well before one can mock it.
@BryanCrotaz
Copy link

@wyze any idea why we're all getting different sets of test failures with the same source?

* updated tests caches

* updated hash in test
@gaving
Copy link

gaving commented Dec 30, 2016

I'm desperate for this functionality so tried installing this pull with:-

npm install https://github.com/yarnpkg/yarn#pull/1816/head

But just ended up with the same error @epmatsw has posted:-

Step 15 : RUN yarn add git+ssh://[email protected]:test/client-node.git
 ---> Running in 9551d6b4c139
module.js:471
    throw err;
    ^

Error: Cannot find module '../lib-legacy/constants'
    at Function.Module._resolveFilename (module.js:469:15)
    at Function.Module._load (module.js:417:25)
    at Module.require (module.js:497:17)
    at require (internal/module.js:20:19)
    at Object.<anonymous> (/usr/local/lib/node_modules/yarn/bin/yarn.js:9:17)
    at Module._compile (module.js:570:32)
    at Object.Module._extensions..js (module.js:579:10)
    at Module.load (module.js:487:32)
    at tryModuleLoad (module.js:446:12)
    at Function.Module._load (module.js:438:3)
The command '/bin/sh -c yarn add git+ssh://[email protected]:test/client-node.git' returned a non-zero code: 1
ERROR: Build failed: exit code 1

@gurpreetatwal
Copy link

@gaving I also tried pulling as well and got that error at first, but if you manually run npm run build prior to running npm test that error disappears

@BryanCrotaz
Copy link

@wyze any clues as to what we can fix to get this merged?

vpoverennov and others added 7 commits January 1, 2017 04:11
* Silly fix: Node on Ubuntu on Windows crashes when there is a for-of loop followed by an exception

* Run global install script only on CI because it modifies user's environment

* fixed tests setup when fixture location was not set - it would copy full folder otherwise

* Revert "Silly fix: Node on Ubuntu on Windows crashes when there is a for-of loop followed by an exception"

This reverts commit 68179d0.

* cleaned up herlper logic when fixture name is not passed

* refactored test: Unhandled promise warning

* fixed lint error

* fixed lint error

* fixed lint error

* fixed lint error

* moved temp folder creation to an fs function and added a bit to omit folder name collisions
* Adds inquirer libdefs for more exact typings
* Refine / Fix some flowtype errors (mostly in __tests__)
* Fix a small bug in src/util/map.js found by [email protected]
@bestander
Copy link
Member

@f-sign, quite a few tests instability issues were fixed recently, could you rebase?
Ideally after #2370 is merged.

@chrisirhc
Copy link
Contributor

chrisirhc commented Jan 4, 2017

Looks like the error is centered around:

 Error
      Error: Error: Command failed.
      Exit code: 128
      Command: git
      Arguments: clone ssh://[email protected]/substack/node-mkdirp.git /tmp/yarn-install-github-1483469740695-0.6846000772434384/.yarn-cache/.tmp/f87fb05e5feff74616d48fa358e1d0ce
      Directory: /home/travis/build/yarnpkg/yarn
      Output:
      Cloning into '/tmp/yarn-install-github-1483469740695-0.6846000772434384/.yarn-cache/.tmp/f87fb05e5feff74616d48fa358e1d0ce'...
      Warning: Permanently added the RSA host key for IP address '192.30.253.113' to the list of known hosts.
      Permission denied (publickey).
      fatal: Could not read from remote repository.
      
      Please make sure you have the correct access rights
      and the repository exists.

Does this change make it so that previous behavior that did: git clone https://github.com/substack/node-mkdirp.git now does git clone ssh://[email protected]/substack/node-mkdirp.git instead? And it's failing (correctly) due to SSH credentials being rejected by github.com ?

This would be difficult to reproduce locally because most folks that cloned this project would have configured their SSH keys with github.com and the tests would be successful locally because of that.

@chrisirhc
Copy link
Contributor

Looked into the above issue and it originates from 477c612#diff-5d246f90b159bd84bbc99f67527b1a0eR373 . The change in src/util/request-manager.js was forcing yarn to use the plain git resolver (which fails on Travis) instead of the http resolver which would pass on Travis.

Rolled the fix into #2384 along with cleaning up the rebase. This should resolve the test failures we're seeing here. Though it seems the tests are still somewhat flaky.

@bestander
Copy link
Member

This one got a bad rebase, merged via #2384.
Closing this one

@bestander bestander closed this Jan 4, 2017
@Gudahtt
Copy link
Member

Gudahtt commented Jan 18, 2017

Does this solve #513 ? I noticed that #513 is still open and links here.

Edit: I see that it's closed now; thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.