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

Add support for usernames in git resolver (Take 2) #934

Merged
merged 1 commit into from
Oct 18, 2016

Conversation

danharper
Copy link
Contributor

@danharper danharper commented Oct 12, 2016

Summary

The Git resolver did not understand usernames in URLs, e.g. git+ssh://[email protected]/... due to it treating any pattern containing @ as being "un-exploded". See 4112050, #513.

For me it was falling through, and calling the npm resolver instead. The package wasn't published on the registry, so it was erroring there. For others, it would error due to version mismatches if it was on the registry.

Instead, it's now simply treating any pattern with no protocol as being invalid. There's no valid npm git URL without a protocol.

Example failing pattern from before:

eslint-config-radweb@git+https://[email protected]/radweb/eslint-config-radweb.git
                    ^               ^

Test plan

Added test, they pass.

Btw I've noticed https://github.com/npm-ml/ocaml.git#npm-4.02.3 times out a lot.

@sebmck
Copy link
Contributor

sebmck commented Oct 14, 2016

Can you please rebase, there's a lot of unnecessary files here

@danharper
Copy link
Contributor Author

Sure, I'll rebase in the morning. The additional files in the second commit
were added by the test runner. I was unsure if they should be committed?

On Fri, 14 Oct 2016, 22:28 Sebastian McKenzie, [email protected]
wrote:

Can you please rebase, there's a lot of unnecessary files here


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#934 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAfLFELy3JB1Ws9uqIqqkR8CMtd-8Phxks5qz_PngaJpZM4KVPuG
.

@danharper
Copy link
Contributor Author

@kittens updated. The second commit still contains the additional test fixtures which Jest produced.

@EugeneHlushko
Copy link

Hi @kittens, can we proceed with this PR? Looking forward to move yarn into few projects but git+ssh url from private git repo is still not found:
error Couldn't find package "HIDDENTEXT" on the "npm" registry.

@danharper
Copy link
Contributor Author

I've rebased the PR with master.

I've also removed the second commit containing all the .bin fixtures generated by Jest. Looking at #1093 it seems these shouldn't be generating, but they're created every time I run tests locally.

@joseph-allen
Copy link

this is the fix I need for my project!

@EugeneHlushko
Copy link

Hi @kittens , can you comment in why this is stalled? Maybe we can help out

@bestander bestander merged commit 41e1d18 into yarnpkg:master Oct 18, 2016
@bestander bestander mentioned this pull request Oct 18, 2016
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.

5 participants