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

Fix support for private GitHub shortcuts deps #971

Closed
wants to merge 1 commit into from
Closed

Fix support for private GitHub shortcuts deps #971

wants to merge 1 commit into from

Conversation

ramasilveyra
Copy link
Contributor

@ramasilveyra ramasilveyra commented Oct 13, 2016 •
edited

Summary

Fix support for private GitHub shortcuts deps. See #573

Test plan

Example of commands and code

Only works with shortcuts:

"private-test": "github:ramasilveyra/private-test#d6c5789"
"private-test": "ramasilveyra/private-test#d6c5789"

I tried to follow the same name convention of npm/hosted-git-info for the "urls".

I found that npm first tries the git:// (info.git()) url then the git@... (info.ssh()) url then the https:// (info.https()) url and finally the git+ssh://... (info.sshurl()) url: npm/npm@6b0f588

Also what do you think about using this package to support the same repository urls for Github, Bitbucket and Gitlab as npm? It could solve a couple of incompatibility issues.

To make it work with this urls needs a little of more work on the GitResolver:

"private-test": "git://github.com/ramasilveyra/private-test.git#d6c5789"
"private-test": "git@github.com:ramasilveyra/private-test.git#d6c5789"
"private-test": "git+https://github.com/ramasilveyra/private-test.git#d6c5789"
"private-test": "git+ssh://git@github.com/ramasilveyra/private-test.git#d6c5789"

Update
yarn v0.16.0 actually works with:

"private-test": "git+ssh://git@github.com/ramasilveyra/private-test.git#d6c5789"

@iddan
Copy link

iddan commented Oct 13, 2016

Much needed!

@palnes
Copy link

palnes commented Oct 13, 2016

Go on, little pull request. you can do it!

@ramasilveyra ramasilveyra force-pushed the fix-private-git-packages branch 2 times, most recently from 0b9d1b5 to 1c9d898 Compare October 15, 2016 01:20
@sjmueller
Copy link

I want to use yarn too! Can't wait til this one is merged :)

@ramasilveyra ramasilveyra changed the title Fix support for private git urls Fix support for private github urls Oct 17, 2016
@ramasilveyra ramasilveyra changed the title Fix support for private github urls Fix support for private GitHub urls Oct 17, 2016
@ramasilveyra ramasilveyra changed the title Fix support for private GitHub urls Fix support for private GitHub shortcuts urls Oct 17, 2016
@ramasilveyra ramasilveyra changed the title Fix support for private GitHub shortcuts urls Fix support for private GitHub shortcuts deps Oct 17, 2016
@BryanCrotaz BryanCrotaz mentioned this pull request Oct 17, 2016
Closed
@danielkcz
Copy link

Exactly. This is currently only issue that is holding me back from embracing Yarn fully :)

@chicoxyzzy chicoxyzzy mentioned this pull request Oct 18, 2016
Closed
danielkcz
danielkcz reviewed Oct 18, 2016
View reviewed changes
src/resolvers/exotics/hosted-git-resolver.js
@@ -71,6 +71,11 @@ export default class HostedGitResolver extends ExoticResolver {
throw new Error('Not implemented');
}

static getGitSSH(exploded: ExplodedFragment): string {
exploded;
throw new Error('Not implemented');
Copy link

Choose a reason for hiding this comment

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

What is actually the purpose of this? Why would you add call to this function and just throw an error in it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is what it does on all the methods of HostedGitResolver (src/resolvers/exotics/hosted-git-resolver.js) https://github.com/ramasilveyra/yarn/blob/efd9801987ef395a6a5ed30b3bc5e0cef94326b5/src/resolvers/exotics/hosted-git-resolver.js#L58-L84
I think that is for dev assurance for when you extend the class HostedGitResolver.

@bestander
Copy link
Member

@ramasilveyra, that is quite awesome but how about a couple of new tests?

@hzoo
Copy link
Contributor

hzoo commented Oct 18, 2016

@bestander would the tests be in package-resolver.js? Looks like they are commented out atm

yarn/__tests__/package-resolver.js

Lines 17 to 18 in 5ce30e8

// TODO renable these test.concurrent
xit(`resolve ${pattern}`, async () => {

@ramasilveyra
Copy link
Contributor Author

ramasilveyra commented Oct 18, 2016 •
edited

@bestander today EOD arg time I will try to add tests. But anyway if someone is eager and want to add it, prs to the branch fix-private-git-packages on my fork are welcome.

@ramasilveyra ramasilveyra force-pushed the fix-private-git-packages branch 2 times, most recently from 3860996 to 2790d04 Compare October 19, 2016 04:01
@ramasilveyra
Copy link
Contributor Author

ramasilveyra commented Oct 19, 2016 •
edited

@bestander

Tests added!

Could you create a dummy private dep onhttps://github.com/yarnpkg/private-dep and give access to that to CircleCI, AppVeyor and TravisCI? Also I need the commit-ish. Thanks!

@ramasilveyra ramasilveyra force-pushed the fix-private-git-packages branch 4 times, most recently from 532afda to 0d3deef Compare October 19, 2016 04:30
@ramasilveyra
Copy link
Contributor Author

@hzoo I tried to enable it. But addTest('github:PolymerElements/paper-elements', 'bower'); // github url always fails, even on master. Maybe is that.

@danielkcz
Copy link

Looks like that version 0.16 already works with private repositories, so this might be obsolete.

@ramasilveyra
Copy link
Contributor Author

@FredyC Tried with yarn v0.16.0 and private github shortcuts without success :(
screen shot 2016-10-19 at 3 11 34 am

Could you provide an example?

@ramasilveyra
Copy link
Contributor Author

ramasilveyra commented Oct 19, 2016 •
edited

@FredyC Just found that yarn v0.16.0 actually works with "private-test": "git+ssh://git@github.com/ramasilveyra/private-test.git#d6c5789" that is good!

@danielkcz
Copy link

Yea, looks like it doesn't like variant with semicolon, found out about that too, but some packages are still failing for me. I have all private packages at GitLab.

@danielkcz
Copy link

@ramasilveyra Can you perhaps rebase to master? I would like to actually check this PR in action if it solves anything from my current issues.

@trashhalo
Copy link

out of date again. and whats the deal with the appveyor failure?

yarn install
'node' is not recognized as an internal or external command,
operable program or batch file.
Command exited with code 1

@ramasilveyra
Copy link
Contributor Author

ramasilveyra commented Oct 19, 2016 •
edited

@trashhalo appveyor is broken for all. From de discord server #general:

Daniel15 - Today at 3:44 AM
@CMDR willyb321 On AppVeyor, you can download and install the msi if Chocolatey doesn't have the newest version (currently we're waiting for the Chocolatey team to approve 0.16.0)
https://www.appveyor.com/docs/how-to/download-file/ you can download https://yarnpkg.com/latest.msi using one of those methods, then msiexec /qn to install it.

Branch updated again :) cc @FredyC

@ramasilveyra ramasilveyra force-pushed the fix-private-git-packages branch 2 times, most recently from 8bf83d7 to 864ad07 Compare October 24, 2016 06:41
@ramasilveyra ramasilveyra force-pushed the fix-private-git-packages branch 3 times, most recently from 5681b82 to 7e73e44 Compare October 27, 2016 02:20
@bestander
Copy link
Member

Thanks for waiting, I am ready to merge this.
So what is the state of this feature?
@ramasilveyra could you rebase on master and explain how what feature it adds compared to fixes in 0.16?

bestander
bestander approved these changes Oct 28, 2016
View reviewed changes
__tests__/package-resolver.js
@@ -12,6 +12,7 @@ import * as fs from '../src/util/fs.js';
jasmine.DEFAULT_TIMEOUT_INTERVAL = 60000;

const path = require('path');
const isCI = require('is-ci');
Copy link
Member

Choose a reason for hiding this comment

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

nice find!

@bestander bestander self-assigned this Oct 28, 2016
bestander
bestander requested changes Oct 28, 2016
View reviewed changes
Copy link
Member

@bestander bestander left a comment

Choose a reason for hiding this comment

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

rebase

@ramasilveyra
Copy link
Contributor Author

ramasilveyra commented Oct 28, 2016 •
edited

@bestander branch rebased.

Remember that now the __tests__/package-resolver.js has some tests for private deps. We need a dummy private dep on https://github.com/yarnpkg/private-dep and give access to that to CircleCI, AppVeyor and TravisCI and also replace the commit ish on lines 62 and 63:
https://github.com/yarnpkg/yarn/pull/971/files#diff-acf7209ee8a640217777103218222465R62

Also the tests of package-resolver.js for some reason are disabled. Do you know why?

This feature adds support for private GitHub shortcuts ("private-test": "github:ramasilveyra/private-test#d6c5789" or "private-test": "ramasilveyra/private-test#d6c5789").
v0.16 only supports private deps in this form: "private-test": "git+ssh://git@github.com/ramasilveyra/private-test.git#d6c5789".

@ramasilveyra
Copy link
Contributor Author

ramasilveyra commented Oct 31, 2016 •
edited

Closed because support was added by #1081 and It's getting complicated to keep this pr updated with master.

Update
I was wrong #1081 didn't fix the issue.

@ramasilveyra ramasilveyra deleted the fix-private-git-packages branch October 31, 2016 14:54
@martynchamberlin martynchamberlin mentioned this pull request Nov 1, 2016
Closed
@psyrendust
Copy link

@ramasilveyra Did this ever get resolved for private GitHub shortcuts deps? I'm wondering based on your last comment: #971 (comment)

I'm using yarn version 0.21.3 and I'm still having issues. I'm getting the following error with 2 private repos hosted on github, which works fine with npm:

"contextual-search": "westfield/contextual-search#v1.3.2",
"maps-cw-fixtures": "westfield/maps-cw-fixtures#v0.1.0",
Trace: 
  Error: Error connecting to repository. Please, check the url.
      at /usr/local/Cellar/yarn/0.21.3/libexec/lib/node_modules/yarn/lib/resolvers/exotics/hosted-git-resolver.js:162:15
      at next (native)
      at step (/usr/local/Cellar/yarn/0.21.3/libexec/lib/node_modules/yarn/node_modules/babel-runtime/helpers/asyncToGenerator.js:17:30)
      at /usr/local/Cellar/yarn/0.21.3/libexec/lib/node_modules/yarn/node_modules/babel-runtime/helpers/asyncToGenerator.js:28:13
      at process._tickCallback (internal/process/next_tick.js:103:7)

I've even tried the ~./ssh/config hack listed here: #513 (comment)

@anonrig
Copy link

anonrig commented Apr 4, 2017

I'm having the same issue. Since I have 2FA enabled in Github I have to use git+ssh on my private github repos. I'm getting Error connecting to repository. Please, check the url. error. Anybody have any solution? Or is this a big lack of feature provided by Yarn itself?

@nickfujita
Copy link

@psyrendust @anonrig Were either of you able to get this working for private git repos? I also have 2FA enabled on github.

@anonrig
Copy link

anonrig commented Apr 13, 2017

I shouldn't and forced to use NPM unfortunately. @nickfujita

@psyrendust
Copy link

@nickfujita No, I'm not able to get this working with my orgs private repo's. I'm also using 2FA.

@MiguelMadero
Copy link
Contributor

MiguelMadero commented Apr 13, 2017 via email

@ramasilveyra
Copy link
Contributor Author

ramasilveyra commented May 6, 2017 •
edited

FYI just tested the version from master, and looks like the support for private git repos (with shorted syntax) works great!

Kudos for @thedumbterminal #2992 💥

It was released on v0.24.0: https://github.com/yarnpkg/yarn/releases/tag/v0.24.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielkcz danielkcz danielkcz left review comments

@bestander bestander bestander requested changes

Assignees

@bestander bestander

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet