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

Yarn 1.X - Can't install Github Pull Requests anymore #4369

Closed
mtraynham opened this issue Sep 8, 2017 · 6 comments · Fixed by #4411
Closed

Yarn 1.X - Can't install Github Pull Requests anymore #4369

mtraynham opened this issue Sep 8, 2017 · 6 comments · Fixed by #4411
Assignees

Comments

@mtraynham
Copy link
Contributor

Do you want to request a feature or report a bug?
Bug

What is the current behavior?
Yarn 0.X allowed you to install PR commits by referencing the PR number. I've seen a few ways to do this, but it's always worked for me as:
yarn add angular-ui/ui-layout#228/head

Using 1.X, you now get:

yarn add v1.0.1
info No lockfile found.
[1/4] Resolving packages...
error Couldn't find match for "228/head" in "_c" for "https://github.com/angular-ui/ui-layout.git".
info Visit https://yarnpkg.com/en/docs/cli/add for documentation about this command.

If the current behavior is a bug, please provide the steps to reproduce.
yarn init; yarn add angular-ui/ui-layout#228/head

What is the expected behavior?
It installs the head commit of the Pull Request.

Please mention your node.js, yarn and operating system version.
Node 8.4
Yarn 1.0.1
Linux Mint 18.3

@KristofMorva
Copy link

I can confirm. I'd also like add, it's not working for getting PR commit IDs either (like flatlogic/awesome-bootstrap-checkbox#49f4bd03155c6ce6c006d0957c22e65888d657a9).

@kaylie-alexa kaylie-alexa self-assigned this Sep 11, 2017
@BYK BYK closed this as completed in #4411 Sep 12, 2017
BYK pushed a commit that referenced this issue Sep 12, 2017
**Summary**

Fixes #4369, a regression from previous refactor.
Include `pull` requests in ref search and test for it later when it's called by github resolver.

*BEFORE*
![before](https://user-images.githubusercontent.com/18429494/30309691-7aeabe2e-9741-11e7-8e34-c75413b83d99.png)


*AFTER*
![after](https://user-images.githubusercontent.com/18429494/30309693-7dcc3528-9741-11e7-8a74-bc7585fd6177.png)

**Test plan**

Added a unit test in `git/git-ref-resolver.js`
@BYK
Copy link
Member

BYK commented Sep 12, 2017

@mtraynham @KristofMorva please see @arcanis' comment on the PR: #4411 (comment)

Looks like it was intentional removing the PR support and I now remember having a similar discussion now. The reason was the /pr/ pattern looks like the code is from this repo but it can be from another repo which may be a security issue. Instead of using PR's directly, it is still possible to use the different repo + commit combination. What do you think about this?

@mtraynham
Copy link
Contributor Author

mtraynham commented Sep 12, 2017

I don't disagree, but I think you guys are missing a valuable feature of PRs when used this way; at least from my interpretation it seems like Github has implemented it this way.

When a forked repo branches and creates a PR, referencing a forked branch is circumstance to being deleted and the git commit reference or branch alias could disappear (e.g. yarn add mtraynham/ui-layout#my_branch). When referencing a PR "branch" in the upstream repo, the user could delete the forked branch, but the commits within the PR persist and they cannot be deleted (e.g. yarn add angular-ui/ui-layout#228/head).

@KristofMorva
Copy link

@BYK yes, it's true we can. For us, it's just more obvious that "we are waiting for this PR to be merged" instead of referencing a forked repo, and it's also easier to check if we can get back to the original repository because on the PR you see it's status, the fork is really uninformative. But it's only our case, we can adapt if this change was intentional :)

@BYK
Copy link
Member

BYK commented Sep 12, 2017

Thank you both for the very informative answers!

When a forked repo branches and creates a PR, referencing a forked branch is circumstance to being deleted and the git commit reference or branch alias could disappear (e.g. yarn add mtraynham/ui-layout#my_branch). When referencing a PR "branch" in the upstream repo, the user could delete the forked branch, but the commits within the PR persist and they cannot be deleted (e.g. yarn add angular-ui/ui-layout#228/head).

I think this is a good-enough reason to keep it, what do you think @arcanis?

joaolucasl pushed a commit to joaolucasl/yarn that referenced this issue Oct 27, 2017
**Summary**

Fixes yarnpkg#4369, a regression from previous refactor.
Include `pull` requests in ref search and test for it later when it's called by github resolver.

*BEFORE*
![before](https://user-images.githubusercontent.com/18429494/30309691-7aeabe2e-9741-11e7-8e34-c75413b83d99.png)


*AFTER*
![after](https://user-images.githubusercontent.com/18429494/30309693-7dcc3528-9741-11e7-8a74-bc7585fd6177.png)

**Test plan**

Added a unit test in `git/git-ref-resolver.js`
@vizath
Copy link

vizath commented Feb 26, 2018

Note to future Google searches: I had permission issues on my ssh key which also gave this error.

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 a pull request may close this issue.

5 participants