Support GitHub Enterprise #319
Conversation
|
Sorry to bother. Thanks for your feedback! |
|
|
||
| boolean isGitHubUrl = false; | ||
| try{ | ||
| GitHubOptions githubOptions = options.get(GitHubOptions.class); |
There was a problem hiding this comment.
could we move this outside of the try catch block?
| GitHubOptions githubOptions = options.get(GitHubOptions.class); | ||
| GitHubHost ghHost = githubOptions.getGitHubHost(url); | ||
| isGitHubUrl = ghHost.isGitHubUrl(url); | ||
| } catch (EvalException e) { |
There was a problem hiding this comment.
empty catch blocks are a bit of an anti-pattern, and I'd like to avoid having these if possible. is there any way you can refactor this into a method in GitHubOptions that returns whether a url belongs to one of the GH hosts?
if you must keep the empty catch block, please leave a comment explaining why we catch and swallow the exception
| !ghHost.isGitHubUrl(url), | ||
| "Git origins with github should use github approval providers!"); | ||
| } catch(EvalException e){ | ||
| // nothing to-do, apparently this is no GitHub URL. |
There was a problem hiding this comment.
same comments as above
| public GitHubApiTransportImpl(GitRepository repo, HttpTransport httpTransport, | ||
| String storePath, boolean bearerAuth, Console console) { | ||
| public GitHubApiTransportImpl(GitHubHost ghHost, GitRepository repo, HttpTransport httpTransport, | ||
| String storePath, boolean bearerAuth, Console console) { |
There was a problem hiding this comment.
nit: extra whitespace
There was a problem hiding this comment.
can we add a test that checks if a origin URL is considered a valid GitHub URL when that host is set as a GitHub host?
|
Great, I will try to solve that in the next days! |
castler
force-pushed
the
support_github_enterprise
branch
from
c3f8282 to
3f87086
Compare
GitHub can be hosted by companies for their own usage (under their own URL). This changset removes the hard-coded url to `github.com` and makes it confligurable for a user, which GitHub-instances shall be supported. Closes google#134
| names = "--github-allowed-hosts", | ||
| description = "If using GitHub Enterprise, one needs to specify valid hosts. By default only `github.com` is supported." | ||
| ) | ||
| public List<String> gitHubAllowedHosts = ImmutableList.of("github.com"); |
There was a problem hiding this comment.
Thinking about this more, I'm realizing that a flag to set the "allowed" GitHub hosts is not going to scale well for our internal use, and it might not be user friendly for our OSS users. We should avoid using a flag for setting the host allowlist.
I see why the flag was needed in the first place: git.origin was allowed to accept GitHub use cases, which in retrospect was not ideal. However, we also have git.github_origin (GitHub specific) in the module, which gives me an idea.
What if we make it so that git.github_origin assumes that the host in the URL is a valid GitHub host? i.e., if a user says the repo URL is foo.com, then we just treat it as if it has GitHub APIs?
As for the standard git.origin, github.com can be considered the only valid GitHub host. Therefore, Github enterprise specific origins should use git.github_origin
In this way, we are:
- leveraging the user's intent in choosing
github_originto set it as a valid GitHub host as that is the most likely intent - preserving the existing behavior for legacy
git.originusers.
what do you think? please LMK if you have any questions, and thank you again for working on this.
There was a problem hiding this comment.
Understood - I am afraid of cases where no github specific function is available. I am not that familiar yet with CopyBara in order to judge this out of my head if such cases exist.
So I will give this a try in the next days.
GitHub can be hosted by companies for their own usage (under their own URL).
This changset removes the hard-coded url to
github.comand makes it confligurable for a user, which GitHub-instances shall be supported.Closes #134