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

Add host argument to github_resolve_ref() #285

Closed

Conversation

yutannihilation
Copy link
Contributor

Fix #284

Currently, github_resolve_ref() doesn't pass host to github_GET(), so if the ref contains a branch or a PR name, it fails for GitHub Enterprise.

This PR adds host argument to github_resolve-ref() and pass host to it.

@yutannihilation
Copy link
Contributor Author

Note that, though I believe this is the right change to make, I cannot use devtools::install_github() without dependencies = FALSE for a GitHub Enterprise that requires PATs. We can set GITHUB_PAT for only one of the GitHub Enterprise or github.com, which means the other one fails with 401 error (bad credentials)...

I almost gave up. No idea what to do to make remotes do the right thing.

@jimhester
Copy link
Member

I don't think we can support multiple PATs without serious refactoring of the internals. I guess the assumption is if you are using github enterprise all of the github remotes should live in your enterprise instance.

@yutannihilation
Copy link
Contributor Author

I guess the assumption is if you are using github enterprise all of the github remotes should live in your enterprise instance.

Thanks, agreed. Then, I'll try to make the same change on other places.

By the way, how about using option() instead of host argument? I feel it's a bit error-prone to pass them as arguments.

@yutannihilation
Copy link
Contributor Author

@jimhester
Sorry, I gave this up since I found users often need to use both GitHub.com (for installing dev packages) and GitHub Enterprise (for installing internal packages) at the same time.

The problem is that install_github() checks updates for all installed packages, but some of my packages are installed from GitHub.com even when I try to install a package from GitHub Enterprise, so I get 404 errors.

So, I really need multiple PATs. On the other hand, I think this is right.

I don't think we can support multiple PATs without serious refactoring of the internals.

remotes package handles pretty complex things, so I understand that it's a reasonable decision not to support multiple PAT. I may come up with some workaround for this, but it will probably make things more complicated. So, let me close this.

(I don't intend to blame you, but just want to let you know currently it's very difficult to use install_github() for GitHub Enterprise...)

@yutannihilation yutannihilation deleted the fix/github-resolve-ref branch January 30, 2019 10:26
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.

github_resolve_ref() should take host
2 participants