-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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 GitHub Enterprise Server support #7570
base: main
Are you sure you want to change the base?
Add GitHub Enterprise Server support #7570
Conversation
- Add TypeScript interface declarations for GitHub-related global window properties - Fix TypeScript build errors related to missing window property types - Ensure proper type checking for GitHub Enterprise integration
- Fix trailing whitespace in GitHub auth URL and URL parsing test files - Add proper end-of-file newlines to test files - Improve code formatting consistency in test files
- Add proper end-of-file newlines to Python files - Fix import ordering in test files - Standardize string quote style in GitHub configuration files - Improve code formatting and line wrapping in GitHub service tests
- Fix import ordering and remove unused imports - Improve function parameter formatting for better readability - Standardize string quote style in GitHub and settings related files - Fix whitespace and indentation issues - Remove unnecessary blank lines
Thanks a bunch for putting this together! The main feedback I have is
We're trying to make this configurable on a per-user; let me know if you need any help with this! |
@malhotra5 |
Yup its so that the changes can be ported over to SAAS as well Its also a matter of consistency, as for self hosted openhands we still maintain values inside user settings rather than setting values globally |
This commit adds support for GitHub Enterprise Server instances: - Add base_url parameter to GitService interface - Add host_url field to ProviderToken model - Enhance GitHubService to handle custom API URLs for Enterprise Server - Update settings models and routes to handle host_url information - Add Enterprise Host URL input field to account settings UI - Update tests to validate Enterprise Server functionality These changes enable users to configure and connect to their own self-hosted GitHub Enterprise Server instances through the UI, allowing each user to specify their own custom GitHub host. Backward compatibility with github.com is maintained.
c7a108c
to
98adb9f
Compare
I've implemented the changes you suggested. Here's what I've done:
These changes allow users to configure their GitHub Enterprise Server host URL on a per-user basis, maintaining consistency with other OpenHands settings that are managed at the user level. For self-hosted environments, I've maintained the ability to configure global settings through environment variables (GITHUB_ENTERPRISE_URL) and configuration files. The implementation prioritizes the user-specific host_url when available, falling back to the globally configured URL when not set. Please let me know if you'd like to see any additional changes or have any questions! |
…rise-server-support
Thank you for reviewing this PR. Since the last review, I have added new commits, but in the meantime, GitLab-related changes were introduced, requiring me to resolve conflicts twice. Given that the time available for this PR is limited, I would like to discuss the best way to proceed smoothly. For example, one possible approach could be merging the PR in its current state and handling any necessary fixes in a separate PR. Could you share your thoughts on this? I appreciate your help. |
Thanks for running the CI! It looks like it failed, so I’ll go ahead and push a fix to get it passing. |
@doew I think, if you run a |
export const initGitHubEnterpriseConfig = () => { | ||
// Set GitHub Enterprise URL if available | ||
if (import.meta.env.VITE_GITHUB_ENTERPRISE_URL) { | ||
window.GITHUB_ENTERPRISE_URL = import.meta.env.VITE_GITHUB_ENTERPRISE_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.
Could we avoid using window
to pass around GitHub related settings. We'd rather just use the UserSettings
because its persists for local installations as well
|
||
// Extend Window interface to include GitHub Enterprise URL | ||
declare global { | ||
interface Window { |
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.
similarly remove use of window here
@@ -18,7 +32,7 @@ export type Settings = { | |||
ENABLE_DEFAULT_CONDENSER: boolean; | |||
ENABLE_SOUND_NOTIFICATIONS: boolean; | |||
USER_CONSENTS_TO_ANALYTICS: boolean | null; | |||
PROVIDER_TOKENS: Record<Provider, string>; | |||
PROVIDER_TOKENS: Record<Provider, ProviderTokenDataGet | string>; |
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 enforce this to always be ProviderTokenData
instead of `string
Also the backend route can end empty token for GET
requests, instead of making ProviderTokenDataPost
and ProviderTokenDataGet
@@ -7,6 +7,19 @@ | |||
*/ | |||
export const generateGitHubAuthUrl = (clientId: string, requestUrl: URL) => { | |||
const redirectUri = `${requestUrl.origin}/oauth/keycloak/callback`; | |||
|
|||
// Check if GitHub Enterprise Server URL is configured | |||
const githubEnterpriseUrl = window.GITHUB_ENTERPRISE_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.
can we have a param from the user settings passed to this function instead of pulling the URL value out from window
) | ||
else: | ||
# Default to github.com API | ||
self.BASE_URL = get_github_api_url() or self.DEFAULT_BASE_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.
NIT: lets define the helper functions within this class (e.g get_github_api_url
)
Let's remove the config class and just use user settings for storing the BASE_URL
values
@@ -93,9 +96,14 @@ async def get_github_installation_ids( | |||
): | |||
if provider_tokens and ProviderType.GITHUB in provider_tokens: | |||
token = provider_tokens[ProviderType.GITHUB] | |||
# Use the host_url from the token if available, otherwise use the default API URL | |||
github_api_url = token.host_url or get_github_api_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.
We should have host_url information available in the token, so get_github_api_url
should be unnecessary
We strictly want to store such information on a per user basis
FYI #7766 is adding similar support but for Gitlab, so some of the scaffolding changes may be quite similar |
Thank you very much for the helpful review! |
End-user friendly description of the problem this fixes or functionality that this introduces.
This update introduces support for GitHub Enterprise Server (GHES), allowing OpenHands to integrate seamlessly with self-hosted GitHub environments. Users can now configure OpenHands to connect with GHES instances through updated API endpoints and improved authentication mechanisms.
Give a summary of what the PR does, explaining any non-trivial design decisions.
This PR adds support for GitHub Enterprise Server by:
The design ensures that these changes are backward-compatible with existing GitHub.com integrations.
Link of any specific issues this addresses.
N/A