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

Allow detecting repository on GitHub Enterprise #147

Merged
merged 1 commit into from
Jan 2, 2019
Merged

Allow detecting repository on GitHub Enterprise #147

merged 1 commit into from
Jan 2, 2019

Conversation

mrmanc
Copy link
Contributor

@mrmanc mrmanc commented Dec 12, 2018

Alters the regex that extracts the org/username and repo name from the repo address based on suggestion by @benbalter, so that it can cope with repos that do not exist on github.com (e.g. GitHub Enterprise).

Adds a test for this change.

Resolves #120

Alters the regex that extracts the org/username and repo name from the repo address, so that it can cope with repos that do not exist on github.com (e.g. GitHub Enterprise).

Adds a test for this change.
@ashmaroli ashmaroli changed the title Fix #120 by adapting the GitHub repo regex Allow detecting repository on GitHub Enterprise Dec 12, 2018
@ashmaroli ashmaroli requested a review from benbalter December 12, 2018 17:56
@mrmanc
Copy link
Contributor Author

mrmanc commented Dec 13, 2018

In addressing why I was getting local failures (#148) I found that the rspec expects looked wrong here, and have resolved this. I think I have also resolved #148. Would you be open to me adding those changes to this PR or would you prefer I open a new one? There’s a little overlap as I’ve copied the inaccurate expects convention in the commits for this PR already, and I’d like to tidy that up.

@parkr
Copy link
Member

parkr commented Jan 2, 2019

I believe the AppVeyor failure is a flake (a failed network connection, perhaps?).

Let's ship this as-is, and add further changes in a separate PR? Feel free to ping me directly, @mrmanc. Thank you!

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit e58d255 into jekyll:master Jan 2, 2019
jekyllbot added a commit that referenced this pull request Jan 2, 2019
@mrmanc
Copy link
Contributor Author

mrmanc commented Jan 3, 2019

Awesome, thanks for this. I’ll sort out a separate PR shortly. Apologies for pinging Ben—I thought it best as he’d suggested the fix. I’ll be sure to aim it at you next time :)

mrmanc added a commit to autotraderuk/github-metadata that referenced this pull request Jan 4, 2019
Tests were coupled to a specific origin meaning that tests against forks would fail. I’ve replaced the test that checks the output of `git_remote_url` is okay into a test to check `git_remotes` returns something, and another to check that `git_remote_url` correctly interprets the output from `git_remotes`.

I’ve also corrected the test doubles in later tests where (as part of jekyll#147) I had copied an incorrect bit of stubbing of `git_remote_url`. The stubbing appeared to be mimicking `git_remotes` instead. The test seemed to be passing by coincidence before. The stubbed output now matches what the function actually does.
@jekyll jekyll locked and limited conversation to collaborators Jan 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

repo detection does not work with github enterprise
4 participants