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

Supports public and private git repos when using shorted repo syntax #2992

Merged
merged 2 commits into from
Apr 6, 2017
Merged

Supports public and private git repos when using shorted repo syntax #2992

merged 2 commits into from
Apr 6, 2017

Conversation

thedumbterminal
Copy link
Contributor

@thedumbterminal thedumbterminal commented Mar 26, 2017

Summary

When using the shortened git syntax such as github:thedumbterminal/grunt-gemnasium#vx, yarn tests for HTTP access using a generated URL ending in .git this will always return a 301 status even if the user does not have permission to access the repo (private repos would require auth first).

This change now uses a full git URL which does not end in .git this means that private repos will fail with 404 so then SSH auth is attempted instead, as long as the correct SSH keys are setup in the users terminal access to the private repo will succeed.

Addresses existing issue: #2774

Test plan

Added new unit tests for the new getGitHTTPBaseUrl method.

This can be manually tested in the following way:

  1. Create a new nam project with: mkdir /tmp/example; cd /tmp/example; npm init -y
  2. Update package.json to add a public repo to dependencies manually in the format of "grunt-gemnasium": "github:thedumbterminal/grunt-gemnasium#v1.1.1"
  3. Update package.json to add a private repo to dependencies manually in the same format, (make sure you can only access it via SSH creds).
  4. Run: yarn --verbose, both modules should be installed.

@thedumbterminal thedumbterminal changed the title Now supports public and private shortened git repos Supports public and private git repos when using shorted repo syntax Mar 26, 2017
@thedumbterminal
Copy link
Contributor Author

@bestander any thoughts on the approach taken here?

@bestander
Copy link
Member

so far looks reasonable, go on :)

@thedumbterminal
Copy link
Contributor Author

Cheers will finish this up hopefully in the week.

const fragment: ExplodedFragment = {
user: 'foo',
repo: 'bar',
hash: '',
};

const expected = 'git+ssh://[email protected]/' + fragment.user + '/' + fragment.repo + '.git';
expect(GitHubResolver.getGitSSHUrl(fragment)).toBe(expected);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously the getGitHTTPUrl test was actaully running getGitSSHUrl instead!

const fragment: ExplodedFragment = {
user: 'foo',
repo: 'bar',
hash: '',
};

const expected = 'git+ssh://[email protected]/' + fragment.user + '/' + fragment.repo + '.git';
expect(GitLabResolver.getGitSSHUrl(fragment)).toBe(expected);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously the getGitHTTPUrl test was actaully running getGitSSHUrl instead!

@thedumbterminal
Copy link
Contributor Author

CI seems to be failing on random tests unrelated to my changes, this work is now ready for someone to review.

@bestander bestander merged commit 2bc0d96 into yarnpkg:master Apr 6, 2017
@bestander
Copy link
Member

Awesome work, @thedumbterminal!

@gsklee
Copy link
Contributor

gsklee commented Jun 13, 2017

Just tried it and it doesn't work (github repo shorthand with private repo); it prompted me to enter my ssh key's passphrase but I cannot actually enter anything at all.

@thedumbterminal
Copy link
Contributor Author

@gsklee You need to make sure you have your ssh key setup in your terminal first, if you are using linux or mac osx then you can run:

eval `ssh-agent`
ssh-add

Then try running the yarn command again.

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.

3 participants