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

Fix remote repository connection checking #2102

Merged
merged 6 commits into from Feb 1, 2019
Merged

Conversation

chrismwendt
Copy link
Contributor

Previously, the frontend would neglect to check that the repository is associated with a GitHub external service in addition to checking that the repository hostname is github.com before finding an access token and looking up the repository on GitHub.com via HTTPS.

This adds a check to make sure the repository is associated with the external service before using the token from the external service.

  • @nicksnyder Does this look like an appropriate change to make? You probably have a better understanding of the data model and relationships between repositories, URLs, access tokens, and external services than I do.
  • @slimsag Does this actually fix the problem you saw in the issue below?

Fixes #1856

@codecov-io
Copy link

codecov-io commented Feb 1, 2019

Codecov Report

Merging #2102 into master will decrease coverage by <.01%.
The diff coverage is 35.29%.

Impacted Files Coverage Δ
cmd/frontend/backend/repos_mock.go 0% <ø> (ø) ⬆️
cmd/frontend/backend/repos_vcs.go 36.11% <27.27%> (-1.15%) ⬇️
cmd/frontend/backend/repos.go 28.71% <50%> (ø) ⬆️

Copy link
Contributor

@nicksnyder nicksnyder left a comment

Choose a reason for hiding this comment

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

Your explanation makes sense and this propose change generally looks correct to me.

Copy link
Contributor

@tsenart tsenart left a comment

Choose a reason for hiding this comment

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

Thanks for helping with this!

cmd/frontend/backend/repos.go Outdated Show resolved Hide resolved
cmd/frontend/backend/repos.go Outdated Show resolved Hide resolved
cmd/frontend/backend/repos.go Outdated Show resolved Hide resolved
cmd/frontend/backend/repos_vcs.go Outdated Show resolved Hide resolved
@chrismwendt
Copy link
Contributor Author

Thanks for the suggestions and review 🙇

@chrismwendt chrismwendt merged commit 5ad5349 into master Feb 1, 2019
@chrismwendt chrismwendt deleted the git-connection branch February 1, 2019 23:40
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.

Repository "Check connection" fails on SSH clones, even though cloning the repo works
5 participants