-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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 support for private GitHub shortcuts deps #971
Conversation
Much needed! |
Go on, little pull request. you can do it! |
0b9d1b5
to
1c9d898
Compare
I want to use yarn too! Can't wait til this one is merged :) |
1c9d898
to
9f4699b
Compare
9f4699b
to
efd9801
Compare
Exactly. This is currently only issue that is holding me back from embracing Yarn fully :) |
@@ -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'); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
@ramasilveyra, that is quite awesome but how about a couple of new tests? |
@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
|
@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. |
3860996
to
2790d04
Compare
Tests added!Could you create a dummy private dep on |
532afda
to
0d3deef
Compare
@hzoo I tried to enable it. But |
0d3deef
to
bffb986
Compare
Looks like that version 0.16 already works with private repositories, so this might be obsolete. |
@FredyC Tried with yarn v0.16.0 and private github shortcuts without success :( Could you provide an example? |
@FredyC Just found that yarn v0.16.0 actually works with |
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. |
@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. |
bffb986
to
85cd1f7
Compare
out of date again. and whats the deal with the appveyor failure?
|
85cd1f7
to
4b620d8
Compare
@trashhalo appveyor is broken for all. From de discord server #general:
Branch updated again :) cc @FredyC |
8bf83d7
to
864ad07
Compare
5681b82
to
7e73e44
Compare
Thanks for waiting, I am ready to merge this. |
@@ -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'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice find!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rebase
7e73e44
to
a5e631d
Compare
@bestander branch rebased. Remember that now the Also the tests of This feature adds support for private GitHub shortcuts ( |
a5e631d
to
b0c741c
Compare
@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 "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 |
I'm having the same issue. Since I have 2FA enabled in Github I have to use |
@psyrendust @anonrig Were either of you able to get this working for private git repos? I also have 2FA enabled on github. |
I shouldn't and forced to use NPM unfortunately. @nickfujita |
@nickfujita No, I'm not able to get this working with my orgs private repo's. I'm also using 2FA. |
We use 2fa and private repos and it works using ssh and the full URI
…On Thu, Apr 13, 2017 at 1:38 AM Larry Gordon ***@***.***> wrote:
@nickfujita <https://github.com/nickfujita> No, I'm not able to get this
working with my orgs private repo's. I'm also using 2FA.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#971 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAC5HK6oOq71mpMN4p6YXan4ISTOd2kkks5rvcLLgaJpZM4KVlPt>
.
|
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 |
Summary
Fix support for private GitHub shortcuts deps. See #573
Test plan
Only works with shortcuts:
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 thegit@...
(info.ssh()) url then thehttps://
(info.https()) url and finally thegit+ssh://...
(info.sshurl()) url: npm/npm@6b0f588Also 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
:Update
yarn v0.16.0 actually works with: