-
Notifications
You must be signed in to change notification settings - Fork 294
Support GitHub Enterprise #319
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
base: master
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: extra whitespace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
chris-campos
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TY for the contribution! Please see line comments.
|
Great, I will try to solve that in the next days! |
c3f8282 to
3f87086
Compare
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@castler Hello!
I just wanted to follow up on this and how your progress is going on this change. We are really interested in adding this feature into Copybara.
If you can no longer work on it, do you mind if we clone your change and continue work on it? We are also happy to answer any questions you may have.
Thanks again for this great contribution thus far!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chris-campos so sorry that I did not answer...I was now on vacation now for two weeks.
Apparently I got some work at my company todo. So if anybody wants to continue right now - feel free. Otherwise I can try to find some time next week hopefully.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, I will see if I can make the changes I suggested above.
chris-campos
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the code changes! Left one more comment about the added flag, LMK what you think
38c43aa to
b635851
Compare
b635851 to
d7184da
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
To better support GitHub enterprise, the GitHub Origin will just assume any host it's given is a GitHub or Github enterprise host. GitOrigin will continue to accept GitHub.com URLs.
d7184da to
ea48ff4
Compare
|
Hi @castler and @chris-campos, This PR is really important for GitHub Enterprise support in our workflow. It would be great to prioritize resolving the remaining comments and failing checks so it can be merged. Happy to help with testing or questions to get this done. Thanks! |
|
@sainathseelam Hi there! We are now working on this internally. |
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