Skip to content
This repository has been archived by the owner on Mar 7, 2019. It is now read-only.

feat: support bitbucket and gitlab shortcut, support private repo through sshurl #12

Merged
merged 1 commit into from
Jun 29, 2018
Merged

feat: support bitbucket and gitlab shortcut, support private repo through sshurl #12

merged 1 commit into from
Jun 29, 2018

Conversation

3cp
Copy link
Collaborator

@3cp 3cp commented Jun 28, 2018

Removed usage of Github api, normalised all hosts (Github/Bitbucket/Gitlab) using git/https/ssh url.
By using sshurl, private repo now works as long as user has ssh public key setup properly on host account.
Patched Github tarball url, can be removed after npm/hosted-git-info#34.

@3cp
Copy link
Collaborator Author

3cp commented Jun 28, 2018

Somehow, travis-cli failed to see public repo

(node:2462) UnhandledPromiseRejectionWarning: Error: Command failed: git ls-remote --refs git+ssh://git@github.com/zkochan/is-negative.git master
Permission denied (publickey).
fatal: Could not read from remote repository.

@3cp
Copy link
Collaborator Author

3cp commented Jun 28, 2018

@zkochan could you configure a user key on travis-ci, apparently it's even needed for ssh url to work on public repo.

ref: https://stackoverflow.com/questions/15674064/how-to-fix-a-permission-denied-publickey-error-for-a-git-submodule-update-in-t

@zkochan
Copy link
Member

zkochan commented Jun 28, 2018

but it means that everyone will have to make this configuration, who will use git dependencies. Currently, it is not required by npm/yarn

@3cp
Copy link
Collaborator Author

3cp commented Jun 28, 2018

Yes, I am surprised too travis-ci cannot even see public repo. I will investigate bit more.

@3cp
Copy link
Collaborator Author

3cp commented Jun 29, 2018

You are right, I myself have github project using git dep running travis-ci without problem.
The above "configure user key" doesn't work because it only applies to private repo.

I am not exactly sure how npm/yarn install git dep. But I can replicate the issue on travis-ci with another git ls-remote command

https://travis-ci.org/huochunpeng/debug-travis

Interestingly, yarn mocked git command in test.

https://github.com/yarnpkg/yarn/blob/101d48f164ab032c232442bdfd1b707a034ca494/__tests__/util/git.js#L3-L17

@3cp
Copy link
Collaborator Author

3cp commented Jun 29, 2018

I got idea why npm/yarn don't have problem with installing git dep on travis-ci.

When npm install git dep, it tries git url and https url before ssh url.
Both git and https url works on travis-ci. Keep in mind, only ssh url works with private repo transparently.

I guess yarn did similar thing.

test('github-shortcut', function (t) {
  const cloneUrls = [
    ['git://github.com/foo/private.git', 'GitHub shortcuts try git URLs first'],
    ['https://github.com/foo/private.git', 'GitHub shortcuts try HTTPS URLs second'],
    ['ssh://git@github.com/foo/private.git', 'GitHub shortcuts try SSH third']

@zkochan how you think we replicate the behaviour? Try git/https/ssh url in sequence?

Performance wise, git url is fastest. I guess it's due to no encryption.

⋊> ~/g/debug-travis on master ◦ time git ls-remote --refs git://github.com/huochunpeng/ast-matcher.git master
        1.39 real         0.00 user         0.01 sys
⋊> ~/g/debug-travis on master ◦ time git ls-remote --refs https://github.com/huochunpeng/ast-matcher.git master
        2.73 real         0.04 user         0.02 sys
⋊> ~/g/debug-travis on master ◦ time git ls-remote --refs git+ssh://git@github.com/huochunpeng/ast-matcher.git master
        6.17 real         0.04 user         0.02 sys

@3cp
Copy link
Collaborator Author

3cp commented Jun 29, 2018

@zkochan There is something I don't like about https path.

When installing private repo, npm pops up a dialog to ask for username/password. Apparently it is trying https url. If I cancel the login, npm still got my private repo installed, apparently it falls back to ssh url which works.

NOT sure whether yarn tried https url before ssh url. From what I can see in yarn.lock file, yarn uses ssh url, not https url. Since yarn mocked git commands in test, it doesn't have issue in ci.

Shall we only try git url and ssh url?

Update: from what I see from yarn verbose ouput, it seems yarn tries http HEAD request on the url before tries git ls-remote, That's how it avoid the login dialog asked by git ls-remote.

I am going to do the same thing.

@3cp
Copy link
Collaborator Author

3cp commented Jun 29, 2018

The test doesn't cover private repo. I guess we have to use mock in test if wanna cover private repo.

But I tested private repo with my local pnpm with patched git-resolver/lib files.

@zkochan
Copy link
Member

zkochan commented Jun 29, 2018

@andreineculau you might want to check this. You worked a lot with this repo

Copy link
Member

@zkochan zkochan left a comment

Choose a reason for hiding this comment

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

LGTM. Would be nice to have a test for private repos. But not a blocker

@andreineculau
Copy link
Contributor

andreineculau commented Jun 29, 2018 via email

@3cp
Copy link
Collaborator Author

3cp commented Jun 29, 2018

I made another small change.

When try HTTP HEAD on https://domain/user/project.git, try url without .git. Because with .git ending, both Github and gitlab respond 302 to redirect it to https://domain/user/project. Need to use url without .git ending to properly test visibility of the repo.

BitBucket treats both urls with same response.

I am adding some basic test coverage for gitlab.

@3cp
Copy link
Collaborator Author

3cp commented Jun 29, 2018

Added test coverage on gitlab.
Added one test on private repo.

});

// This test relies on implementation detail.
// current implementation does not try git ls-remote --refs on pref with full commit hash, this fake repo url will pass.
Copy link
Member

Choose a reason for hiding this comment

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

so what makes it private then?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not this one, the one after. The fake repo fails check on both git url and https url, effectively runs the same code path of private repo by using ssh url.

@zkochan zkochan merged commit ce82b3b into pnpm:master Jun 29, 2018
@zkochan
Copy link
Member

zkochan commented Jun 29, 2018

🚢 pnpm v2.10.0

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

Successfully merging this pull request may close these issues.

3 participants