-
Notifications
You must be signed in to change notification settings - Fork 121
Support for github enterprise server(self hosted github) #646
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
Conversation
""" WalkthroughThe changes introduce support for specifying a custom GitHub API domain, enabling integration with GitHub Enterprise or self-hosted GitHub instances. This is achieved by updating backend service constructors and internal logic to accept and propagate an optional Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as ConfigureGithubModalBody
participant AuthUtils as auth.ts
participant BackendAPI as backend services
User->>UI: Enter GitHub token and optional custom domain
UI->>AuthUtils: checkGitHubValidity(token, customDomain)
AuthUtils->>BackendAPI: Request with baseUrl (customDomain or default)
BackendAPI-->>AuthUtils: Respond with validity
AuthUtils-->>UI: Return result
UI->>BackendAPI: linkProvider(token, customDomain)
BackendAPI->>BackendAPI: Use custom domain for API calls
BackendAPI-->>UI: Confirmation
Assessment against linked issues
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code Graph Analysis (1)web-server/src/content/Dashboards/ConfigureGithubModalBody.tsx (6)
🔇 Additional comments (7)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 10
🔭 Outside diff range comments (2)
backend/analytics_server/mhq/service/code/sync/etl_github_handler.py (1)
1-399
: 💡 Verification agent🧩 Analysis chain
Fix Black formatting issues.
The file is flagged by the Black integration pipeline - please run Black to fix the formatting.
🏁 Script executed:
#!/bin/bash # Run black to check formatting issues black --check backend/analytics_server/mhq/service/code/sync/etl_github_handler.pyLength of output: 180
To verify formatting, let’s install Black and re-run the check:
🏁 Script executed:
#!/bin/bash # Install Black formatter and re-check the file pip install black black --check backend/analytics_server/mhq/service/code/sync/etl_github_handler.pyLength of output: 1364
Let’s run Black via Python’s module path to ensure it’s found:
🏁 Script executed:
#!/bin/bash python3 -m black --check backend/analytics_server/mhq/service/code/sync/etl_github_handler.py
Length of output: 225
Please fix Black formatting issues
The file
backend/analytics_server/mhq/service/code/sync/etl_github_handler.py
is not formatted according to Black (Black reports it “would reformat” this file). Please run Black locally to apply the required changes:python3 -m black backend/analytics_server/mhq/service/code/sync/etl_github_handler.pyAfter formatting, commit the updated file.
🧰 Tools
🪛 GitHub Actions: Black Integration
[error] 1-1: Black formatting check failed. File would be reformatted. Run 'black' to fix code style issues.
web-server/src/content/Dashboards/ConfigureGithubModalBody.tsx (1)
103-112
: 🛠️ Refactor suggestionUpdate dependency array for handleSubmission
The dependency array for
handleSubmission
is missingcustomDomain.valueRef
which is used in the function.Add the missing dependency:
}, [ dispatch, enqueueSnackbar, isLoading.false, isLoading.true, onClose, orgId, setError, - token.value + token.value, + customDomain.valueRef ]);
🧹 Nitpick comments (5)
backend/analytics_server/mhq/exapi/github.py (1)
25-32
: Fix trailing whitespace and improve formatting.The GitHub API service constructor has been updated to support custom domains, but there are formatting issues.
- def __init__(self, access_token: str, domain = "https://api.github.com"): + def __init__(self, access_token: str, domain: str = "https://api.github.com"): self._token = access_token self.base_url = self._get_api_url(domain) self._g = Github( - self._token, + self._token, base_url=self.base_url, per_page=PAGE_SIZE )Note the addition of the type hint for
domain
and removal of trailing whitespace.web-server/pages/api/internal/[org_id]/utils.ts (1)
324-327
: Apply URL sanitization to GraphQL endpointUnlike the REST API URL construction which handles potential double slashes, the GraphQL URL construction doesn't have this safeguard.
Add the same safeguard to the GraphQL URL:
const getGitHubGraphQLUrl = async (): Promise<string> => { const customDomain = await getGitHubCustomDomain(); - return customDomain ? `https://${customDomain}/api/graphql` : GITHUB_API_URL; + const url = customDomain + ? `https://${customDomain}/api/graphql` + : GITHUB_API_URL; + return url.replace(/\/+/g, '/'); };web-server/src/utils/auth.ts (2)
42-44
: Add slash normalization for API URL constructionWhen building the API URL path, multiple slashes could be introduced, similar to what's handled in the utils.ts file.
Normalize slashes in URL construction:
- await axios.get(`${baseUrl}/user`, { + await axios.get(`${baseUrl}/user`.replace(/\/+/g, '/'), {
54-55
: Update function documentation and dependency arrayThe
getMissingPATScopes
function signature has changed, but the JSDoc and dependency array in the parent component are not updated.Add JSDoc to describe the new parameter:
+ /** + * Gets the missing PAT scopes for a GitHub token + * @param pat - The personal access token to check + * @param customDomain - Optional custom domain for GitHub Enterprise + * @returns Array of missing scopes + */ export const getMissingPATScopes = async (pat: string, customDomain?: string) => {web-server/src/content/Dashboards/ConfigureGithubModalBody.tsx (1)
74-77
: Add documentation for the custom_domain parameterThe code sends
custom_domain
as part of meta data but there's no documentation about its format requirements.Add a comment explaining the expected format:
return await linkProvider(token.value, orgId, Integration.GITHUB, { + // custom_domain should be a domain name with or without protocol + // e.g., "github.mycompany.com" or "https://github.mycompany.com" custom_domain: customDomain.valueRef.current });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/analytics_server/mhq/exapi/github.py
(1 hunks)backend/analytics_server/mhq/service/code/sync/etl_github_handler.py
(2 hunks)backend/analytics_server/mhq/service/external_integrations_service.py
(1 hunks)backend/analytics_server/mhq/service/workflows/sync/etl_github_actions_handler.py
(2 hunks)web-server/pages/api/internal/[org_id]/utils.ts
(3 hunks)web-server/src/content/Dashboards/ConfigureGithubModalBody.tsx
(6 hunks)web-server/src/utils/auth.ts
(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
web-server/src/utils/auth.ts (1)
web-server/http-server.js (1)
axios
(5-5)
web-server/pages/api/internal/[org_id]/utils.ts (1)
web-server/src/constants/db.ts (1)
Row
(557-560)
🪛 GitHub Actions: Black Integration
backend/analytics_server/mhq/service/code/sync/etl_github_handler.py
[error] 1-1: Black formatting check failed. File would be reformatted. Run 'black' to fix code style issues.
backend/analytics_server/mhq/service/workflows/sync/etl_github_actions_handler.py
[error] 1-1: Black formatting check failed. File would be reformatted. Run 'black' to fix code style issues.
backend/analytics_server/mhq/exapi/github.py
[error] 1-1: Black formatting check failed. File would be reformatted. Run 'black' to fix code style issues.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: All file linting
🔇 Additional comments (14)
backend/analytics_server/mhq/service/external_integrations_service.py (4)
28-29
: Consistent custom domain parameter propagation.The GitHub API service instance is now created with the custom domain parameter, which allows connecting to GitHub Enterprise instances.
36-37
: LGTM: GitHub API service initialized with custom domain.The GitHub API service is properly initialized with the custom domain parameter.
43-44
: LGTM: GitHub API service initialized with custom domain.The GitHub API service is properly initialized with the custom domain parameter.
50-51
: LGTM: GitHub API service initialized with custom domain.The GitHub API service is properly initialized with the custom domain parameter.
backend/analytics_server/mhq/service/code/sync/etl_github_handler.py (1)
392-394
: LGTM: GitHub API service initialized with custom domain.The GitHub API service is properly initialized with both the access token and custom domain.
backend/analytics_server/mhq/service/workflows/sync/etl_github_actions_handler.py (2)
203-204
: LGTM: GitHub API service initialized with custom domain.The GitHub API service is properly initialized with both the access token and custom domain.
1-206
:❓ Verification inconclusive
Fix Black formatting issues.
The file is flagged by the Black integration pipeline - please run Black to fix the formatting.
🏁 Script executed:
#!/bin/bash # Run black to check formatting issues black --check backend/analytics_server/mhq/service/workflows/sync/etl_github_actions_handler.pyLength of output: 193
Apply Black formatting
The CI indicates formatting issues in this file. Please run the following locally and commit the updated file:black backend/analytics_server/mhq/service/workflows/sync/etl_github_actions_handler.pyThen push the reformatted file so the Black check passes.
🧰 Tools
🪛 Ruff (0.8.2)
70-73: Within an
except
clause, raise exceptions withraise ... from err
orraise ... from None
to distinguish them from errors in exception handling(B904)
🪛 GitHub Actions: Black Integration
[error] 1-1: Black formatting check failed. File would be reformatted. Run 'black' to fix code style issues.
backend/analytics_server/mhq/exapi/github.py (6)
34-38
: LGTM: API URL helper method implementation.This helper method correctly handles the differences between GitHub.com and GitHub Enterprise Server API URLs.
52-57
: LGTM: check_pat method is using the base_url properly.The
check_pat
method now correctly uses the base_url when making the API request, which will work with GitHub Enterprise.🧰 Tools
🪛 Ruff (0.8.2)
56-56: Within an
except
clause, raise exceptions withraise ... from err
orraise ... from None
to distinguish them from errors in exception handling(B904)
157-161
: LGTM: get_contributors method is using the base_url properly.The method correctly uses the base_url to construct the GitHub API URL.
209-213
: LGTM: get_org_members method is using the base_url properly.The method correctly uses the base_url to construct the GitHub API URL.
247-255
: LGTM: get_workflow_runs method is using the base_url properly.The method correctly uses the base_url to construct the GitHub API URL.
1-277
:❓ Verification inconclusive
Fix Black formatting issues.
The file is flagged by the Black integration pipeline - please run Black to fix the formatting.
🏁 Script executed:
#!/bin/bash # Run black to check formatting issues black --check backend/analytics_server/mhq/exapi/github.pyLength of output: 156
Install and run Black to fix formatting issues
Our sandbox couldn’t verify formatting because
black
isn’t available here. Please ensure Black is installed in your environment and rerun:pip install black black --check backend/analytics_server/mhq/exapi/github.py black backend/analytics_server/mhq/exapi/github.pyOnce formatting passes, commit the changes.
🧰 Tools
🪛 Ruff (0.8.2)
56-56: Within an
except
clause, raise exceptions withraise ... from err
orraise ... from None
to distinguish them from errors in exception handling(B904)
🪛 GitHub Actions: Black Integration
[error] 1-1: Black formatting check failed. File would be reformatted. Run 'black' to fix code style issues.
web-server/pages/api/internal/[org_id]/utils.ts (1)
55-56
: 🛠️ Refactor suggestionUpdate the function call to use the new custom domain helper functions
This function still uses the hardcoded API URL from line 9, but should leverage the new
getGitHubRestApiUrl
helper function.Update the function to use the helper:
const searchRepoWithURL = async (searchString: string) => { const apiUrl = await getGitHubRestApiUrl(`repos/${searchString}`); const response = await axios.get<GithubRepo>(apiUrl); const repo = response.data; return [ // ... rest of codeLikely an incorrect or invalid review comment.
backend/analytics_server/mhq/service/code/sync/etl_github_handler.py
Outdated
Show resolved
Hide resolved
backend/analytics_server/mhq/service/workflows/sync/etl_github_actions_handler.py
Outdated
Show resolved
Hide resolved
return `${baseUrl}/${path}`.replace(/\/+/g, '/'); | ||
}; |
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.
🛠️ Refactor suggestion
Domain URL needs protocol verification
The code concatenates the custom domain with path segments, but doesn't ensure the custom domain includes a protocol.
Update the URL construction to ensure proper format:
const getGitHubRestApiUrl = async (path: string): Promise<string> => {
const customDomain = await getGitHubCustomDomain();
- const baseUrl = customDomain ? `https://${customDomain}/api/v3` : GITHUB_API_URL;
+ const baseUrl = customDomain
+ ? (customDomain.startsWith('http') ? customDomain : `https://${customDomain}`) + '/api/v3'
+ : GITHUB_API_URL;
return `${baseUrl}/${path}`.replace(/\/+/g, '/');
};
try { | ||
const response = await axios.get('https://api.github.com', { | ||
const response = await axios.get(baseUrl, { |
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.
🛠️ Refactor suggestion
Fix similar issues with URL construction in getMissingPATScopes
The same URL validation and normalization issues exist in this function.
Apply the same improvements:
const baseUrl = customDomain
- ? `${customDomain}/api/v3`
+ ? `${normalizeCustomDomain(customDomain)}/api/v3`
: DEFAULT_GH_URL;
try {
- const response = await axios.get(baseUrl, {
+ const response = await axios.get(baseUrl.replace(/\/+/g, '/'), {
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
try { | |
const response = await axios.get('https://api.github.com', { | |
const response = await axios.get(baseUrl, { | |
// normalize customDomain and ensure no duplicate slashes when calling the API | |
const baseUrl = customDomain | |
? `${normalizeCustomDomain(customDomain)}/api/v3` | |
: DEFAULT_GH_URL; | |
try { | |
const response = await axios.get( | |
baseUrl.replace(/\/+/g, '/'), | |
{ | |
// …original request options… | |
} | |
); | |
// … | |
} catch (err) { | |
// … | |
} |
web-server/src/utils/auth.ts
Outdated
const baseUrl = customDomain | ||
? `${customDomain}/api/v3` | ||
: DEFAULT_GH_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.
🛠️ Refactor suggestion
Add URL validation and normalization for custom domain
The code assumes the custom domain is properly formatted, but there's no validation or normalization.
Add validation and normalizing for the custom domain:
const baseUrl = customDomain
- ? `${customDomain}/api/v3`
+ ? `${normalizeCustomDomain(customDomain)}/api/v3`
: DEFAULT_GH_URL;
// Add this helper function at the bottom of the file
+ function normalizeCustomDomain(domain: string): string {
+ // Remove trailing slashes
+ domain = domain.replace(/\/+$/, '');
+ // Add protocol if missing
+ if (!domain.startsWith('http')) {
+ domain = `https://${domain}`;
+ }
+ return domain;
+ }
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
const baseUrl = customDomain | |
? `${customDomain}/api/v3` | |
: DEFAULT_GH_URL; | |
const baseUrl = customDomain | |
? `${normalizeCustomDomain(customDomain)}/api/v3` | |
: DEFAULT_GH_URL; | |
// …other code… | |
// Add this helper at the bottom of the file | |
function normalizeCustomDomain(domain: string): string { | |
// Remove trailing slashes | |
domain = domain.replace(/\/+$/, ''); | |
// Add protocol if missing | |
if (!domain.startsWith('http')) { | |
domain = `https://${domain}`; | |
} | |
return domain; | |
} |
const handleDomainChange = (e: string) => { | ||
customDomain.set(e); | ||
showDomainError.set(''); | ||
}; |
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.
🛠️ Refactor suggestion
Add validation function for custom domain
The handleDomainChange
function should validate the domain format.
Add validation to ensure the domain is properly formatted:
const handleDomainChange = (e: string) => {
customDomain.set(e);
showDomainError.set('');
+ // Basic validation when a value is entered
+ if (e && !isValidDomain(e)) {
+ showDomainError.set('Please enter a valid domain');
+ }
};
+ // Helper function to validate domain format
+ const isValidDomain = (domain: string): boolean => {
+ // Allow empty value (optional field)
+ if (!domain) return true;
+
+ // Basic domain validation - allow with or without protocol
+ const domainRegex = /^(https?:\/\/)?([a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?\.)+[a-zA-Z]{2,}(\/.*)?$/;
+ return domainRegex.test(domain);
+ };
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
const handleDomainChange = (e: string) => { | |
customDomain.set(e); | |
showDomainError.set(''); | |
}; | |
const handleDomainChange = (e: string) => { | |
customDomain.set(e); | |
showDomainError.set(''); | |
// Basic validation when a value is entered | |
if (e && !isValidDomain(e)) { | |
showDomainError.set('Please enter a valid domain'); | |
} | |
}; | |
// Helper function to validate domain format | |
const isValidDomain = (domain: string): boolean => { | |
// Allow empty value (optional field) | |
if (!domain) return true; | |
// Basic domain validation - allow with or without protocol | |
const domainRegex = /^(https?:\/\/)?([a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?\.)+[a-zA-Z]{2,}(\/.*)?$/; | |
return domainRegex.test(domain); | |
}; |
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.
I don't see any validation of custom domain input in the frontend. What if the user enters the url with https, what if there are paths. You should validate and normalise the input you receive from the end user.
How is this currently being done in gitlab? Do we not validate or extract the domain from the url?
|
||
if not github_domain: | ||
LOG.warn( | ||
f"Custom domain not found for intergration for org {org_id} and provider {UserIdentityProvider.GITLAB.value}" |
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.
typo and wrong enum
f"Custom domain not found for intergration for org {org_id} and provider {UserIdentityProvider.GITLAB.value}" | |
f"Custom domain not found for integration for org {org_id} and provider {UserIdentityProvider.GITHUB.value}" |
@@ -22,11 +22,20 @@ class GithubRateLimitExceeded(Exception): | |||
|
|||
|
|||
class GithubApiService: | |||
def __init__(self, access_token: str): | |||
def __init__(self, access_token: str, domain = "https://api.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.
type annotate domain
variable
This would be Optional[str]
, i think?
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.
Also instead of setting a default str in the argument. It would be cleaner to put it inside _get_api_url
like in the following comment
def _get_api_url(self, domain: str) -> str: | ||
if domain == "https://api.github.com": | ||
return domain | ||
else: | ||
return f"{domain}/api/v3" |
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.
def _get_api_url(self, domain: str) -> str: | |
if domain == "https://api.github.com": | |
return domain | |
else: | |
return f"{domain}/api/v3" | |
def _get_api_url(self, domain: Optional[str]) -> str: | |
if not domain: | |
return "https://api.github.com" | |
else: | |
return f"{domain}/api/v3" | |
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.
This way the linter is also satisfied
def _get_custom_github_domain() -> Optional[str]: | ||
DEFAULT_DOMAIN = "https://api.github.com" | ||
core_repo_service = CoreRepoService() | ||
integrations = core_repo_service.get_org_integrations_for_names( | ||
org_id, [UserIdentityProvider.GITHUB.value] | ||
) | ||
|
||
github_domain = ( | ||
integrations[0].provider_meta.get("custom_domain") | ||
if integrations[0].provider_meta | ||
else None | ||
) | ||
|
||
if not github_domain: | ||
LOG.warn( | ||
f"Custom domain not found for intergration for org {org_id} and provider {UserIdentityProvider.GITLAB.value}" | ||
) | ||
return DEFAULT_DOMAIN | ||
|
||
return github_domain |
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.
Lets define this function in some util file for github. You can create a utils.py
file in the code
directory and define it there once without having to redefine the same function again. and then import it wherever you want to use it
const getGitHubCustomDomain = async (): Promise<string | null> => { | ||
const provider_meta = await db('Integration') | ||
.where('name', Integration.GITHUB) | ||
.then((r: Row<'Integration'>[]) => r.map((item) => item.provider_meta)); | ||
|
||
return head(provider_meta || [])?.custom_domain || null; | ||
}; |
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.
Lets have some error handling here. If the db query fails somehow it will fail silently. At least a log should be there, so that it can get got in the system logs
|
||
const getGitHubRestApiUrl = async (path: string): Promise<string> => { | ||
const customDomain = await getGitHubCustomDomain(); | ||
const baseUrl = customDomain ? `https://${customDomain}/api/v3` : 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.
This is wrong. How is the default rest API url = 'https://api.github.com/graphql'
|
||
const getGitHubGraphQLUrl = async (): Promise<string> => { | ||
const customDomain = await getGitHubCustomDomain(); | ||
return customDomain ? `https://${customDomain}/api/graphql` : 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.
similarly here. GITHUB_API_URL
is an ambiguous name when we have both rest api and graphql api
<FlexBox gap2 col> | ||
<FlexBox alignBase gap1> | ||
Custom domain | ||
</FlexBox> | ||
<TextField | ||
id="github-custom-domain" | ||
onKeyDown={(e) => { | ||
if (e.key === 'Enter') { | ||
e.preventDefault(); | ||
e.stopPropagation(); | ||
e.nativeEvent.stopImmediatePropagation(); | ||
handleSubmission(); | ||
return; | ||
} | ||
}} | ||
error={!!showDomainError.value} | ||
sx={{ width: '100%' }} | ||
value={customDomain.value} | ||
onChange={(e) => handleDomainChange(e.currentTarget.value)} | ||
label={ | ||
isDomainInputFocus.value || customDomain.value | ||
? 'Custom Domain' | ||
: '(Optional)' | ||
} | ||
onFocus={isDomainInputFocus.true} | ||
onBlur={isDomainInputFocus.false} | ||
/> | ||
</FlexBox> |
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 have a placeholder value to show what a custom domain input would look like. See this review
… and set default API URL
…rror handling for custom domain retrieval
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.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
backend/analytics_server/mhq/utils/github.py (1)
56-76
:⚠️ Potential issueFix potential IndexError and typo in warning message.
The function has two issues that need attention:
- It assumes
integrations[0]
exists, which would cause an IndexError if no GitHub integrations are found for the organization.- There's a typo in the warning message: "intergration" should be "integration".
def get_custom_github_domain(org_id: str) -> Optional[str]: DEFAULT_DOMAIN = "https://api.github.com" core_repo_service = CoreRepoService() integrations = core_repo_service.get_org_integrations_for_names( org_id, [UserIdentityProvider.GITHUB.value] ) - github_domain = ( - integrations[0].provider_meta.get("custom_domain") - if integrations[0].provider_meta - else None - ) + github_domain = None + if integrations and len(integrations) > 0: + github_domain = ( + integrations[0].provider_meta.get("custom_domain") + if integrations[0].provider_meta + else None + ) if not github_domain: LOG.warn( - f"Custom domain not found for intergration for org {org_id} and provider {UserIdentityProvider.GITHUB.value}" + f"Custom domain not found for integration for org {org_id} and provider {UserIdentityProvider.GITHUB.value}" ) return DEFAULT_DOMAIN return github_domain
🧹 Nitpick comments (1)
backend/analytics_server/tests/exapi/test_github.py (1)
33-41
: Add tests for additional domain edge cases.While the current test for custom domains is good, consider adding tests for these edge cases:
- Domains without protocol (e.g., "github.example.com" instead of "https://github.example.com")
- Domains with trailing slashes (e.g., "https://github.example.com/")
- Invalid domains or malformed URLs
These tests would strengthen the robustness of the implementation.
Example test for a domain without protocol:
@patch("mhq.exapi.github.Github", new=DummyGithub) def test_domain_without_protocol_prepends_https(self): token = "deadpool" custom_domain = "github.sujai.com" # No protocol service = GithubApiService(access_token=token, domain=custom_domain) # Verify if the implementation handles this correctly # Either by prepending https:// or by validating input expected = f"https://{custom_domain}/api/v3" self.assertEqual(service.base_url, expected) self.assertEqual(service._g.base_url, expected)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
backend/analytics_server/mhq/exapi/github.py
(1 hunks)backend/analytics_server/mhq/service/code/sync/etl_github_handler.py
(2 hunks)backend/analytics_server/mhq/service/workflows/sync/etl_github_actions_handler.py
(2 hunks)backend/analytics_server/mhq/utils/github.py
(2 hunks)backend/analytics_server/tests/exapi/test_github.py
(1 hunks)web-server/pages/api/internal/[org_id]/utils.ts
(4 hunks)web-server/src/content/Dashboards/ConfigureGithubModalBody.tsx
(6 hunks)web-server/src/utils/auth.ts
(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/analytics_server/mhq/service/workflows/sync/etl_github_actions_handler.py
🚧 Files skipped from review as they are similar to previous changes (5)
- backend/analytics_server/mhq/service/code/sync/etl_github_handler.py
- web-server/src/utils/auth.ts
- backend/analytics_server/mhq/exapi/github.py
- web-server/pages/api/internal/[org_id]/utils.ts
- web-server/src/content/Dashboards/ConfigureGithubModalBody.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/analytics_server/tests/exapi/test_github.py (1)
backend/analytics_server/mhq/exapi/github.py (1)
GithubApiService
(24-273)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: All file linting
🔇 Additional comments (3)
backend/analytics_server/mhq/utils/github.py (1)
3-3
: Appropriate import additions.The new imports properly support the added functionality for handling custom GitHub domains. The
Optional
type hint is correctly used in the function signature.Also applies to: 8-9
backend/analytics_server/tests/exapi/test_github.py (2)
1-12
: Good test setup with appropriate mocking.The test file has a well-structured setup with the
DummyGithub
mock class and proper imports. The mock class effectively captures the constructor parameters for verification.
14-32
: Tests cover basic domain handling scenarios.The tests verify essential behaviors for both default and empty domain cases, ensuring the service correctly falls back to the standard GitHub API URL.
Hi @adnanhashmi09, |
|
||
const getGitHubGraphQLUrl = async (): Promise<string> => { | ||
const customDomain = await getGitHubCustomDomain(); | ||
return customDomain ? `https://${customDomain}/api/graphql` : 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.
return customDomain ? `https://${customDomain}/api/graphql` : GITHUB_API_URL; | |
return customDomain ? `https://${customDomain}/api/graphql` : `${GITHUB_API_URL}/graphql`; |
const checkDomainWithRegex = (domain: string) => { | ||
const regex = | ||
/^(https?:\/\/)[a-zA-Z0-9]+([-.][a-zA-Z0-9]+)*\.[a-zA-Z]{2,}(:[0-9]{1,5})?(\/.*)?$/; | ||
return regex.test(domain); | ||
}; |
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.
Must include test for this. Very difficult to understand what this regex does. Not sure what the logic is.
web-server/src/utils/auth.ts
Outdated
@@ -3,6 +3,8 @@ import { isNil, reject } from 'ramda'; | |||
|
|||
import { Integration } from '@/constants/integrations'; | |||
|
|||
const DEFAULT_GH_URL = 'https://api.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.
can have this defined in a constant.ts file as this is being used in multiple places
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.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
web-server/pages/api/internal/[org_id]/__tests__/github.test.ts (1)
1-85
: 🛠️ Refactor suggestionAdd test for organization-specific domains
Based on the PR objectives to support GitHub Enterprise Server, it would be valuable to test that the utility functions correctly handle organization-specific domains.
Add a test case that verifies organization-specific domain resolution:
it('retrieves custom domain for specific organization', async () => { const orgId = 'test-org-123'; const mockMeta = [{ custom_domain: 'org.github.enterprise.com' }]; (db as jest.Mock).mockReturnValue({ where: jest.fn().mockImplementation((field, value) => { expect(field).toBe('org_id'); expect(value).toBe(orgId); return { where: jest.fn().mockReturnThis(), then: jest.fn().mockResolvedValue(mockMeta) }; }) }); const domain = await githubUtils.getGitHubCustomDomain(orgId); expect(domain).toBe('org.github.enterprise.com'); });🧰 Tools
🪛 Biome (1.9.4)
[error] 19-19: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 29-29: Do not add then to an object.
(lint/suspicious/noThenProperty)
♻️ Duplicate comments (3)
web-server/pages/api/internal/[org_id]/__tests__/github.test.ts (3)
27-30
: Avoid using 'then' method in mock objectsSame issue as before with the 'then' property in mock objects.
🧰 Tools
🪛 Biome (1.9.4)
[error] 29-29: Do not add then to an object.
(lint/suspicious/noThenProperty)
55-55
: Test might fail due to typo in DEFAULT_GH_URLThis test would fail because DEFAULT_GH_URL contains a typo ('githb' instead of 'github'). The test is correctly written, but the constant it depends on has an error.
75-75
: Test might fail due to typo in DEFAULT_GH_URLSame issue as before with the typo in DEFAULT_GH_URL affecting test expectations.
🧹 Nitpick comments (3)
web-server/pages/api/internal/[org_id]/__tests__/github.test.ts (3)
17-20
: Avoid using 'then' method in mock objectsThe static analysis tools flagged the use of the 'then' property on objects. Instead of mocking with a 'then' method that returns a Promise, use Jest's mockResolvedValue directly on the entire chain.
Here's a cleaner approach:
-(db as jest.Mock).mockReturnValue({ - where: jest.fn().mockReturnThis(), - then: jest.fn().mockResolvedValue(mockMeta) -}); +(db as jest.Mock).mockReturnValue({ + where: jest.fn().mockReturnThis(), +}); +(db().where().mockResolvedValue(mockMeta));Or alternatively:
-(db as jest.Mock).mockReturnValue({ - where: jest.fn().mockReturnThis(), - then: jest.fn().mockResolvedValue(mockMeta) -}); +(db as jest.Mock).mockImplementation(() => ({ + where: jest.fn().mockImplementation(() => Promise.resolve(mockMeta)) +}));🧰 Tools
🪛 Biome (1.9.4)
[error] 19-19: Do not add then to an object.
(lint/suspicious/noThenProperty)
45-46
: Typo in error log messageThere's a typo in the error message: "Error occured" should be "Error occurred".
-'Error occured while getting custom domain from database:', +'Error occurred while getting custom domain from database:',
1-85
: Add test for URL protocol handlingThe tests cover domain normalization for trailing slashes, but don't verify how the utils handle URLs with or without protocol prefixes.
Consider adding a test case that verifies protocol handling:
it('handles domains with and without protocol prefixes', async () => { // Test with protocol included jest.spyOn(githubUtils, 'getGitHubCustomDomain').mockResolvedValue('https://git.example.com'); let url = await githubUtils.getGitHubRestApiUrl('repos/owner/repo'); expect(url).toBe('https://git.example.com/api/v3/repos/owner/repo'); // Test without protocol jest.spyOn(githubUtils, 'getGitHubCustomDomain').mockResolvedValue('git.example.com'); url = await githubUtils.getGitHubRestApiUrl('repos/owner/repo'); expect(url).toBe('https://git.example.com/api/v3/repos/owner/repo'); });🧰 Tools
🪛 Biome (1.9.4)
[error] 19-19: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 29-29: Do not add then to an object.
(lint/suspicious/noThenProperty)
🛑 Comments failed to post (2)
web-server/src/utils/domainCheck.ts (1)
1-5: 🛠️ Refactor suggestion
Add JSDoc comments and consider expanding domain validation patterns.
The
checkDomainWithRegex
function provides a good foundation for domain validation, but has some limitations:
The current regex doesn't support:
- IP addresses (e.g., "http://192.168.1.1")
- Localhost domains (e.g., "http://localhost:3000")
- Longer or numeric TLDs (e.g., ".cloud", ".io1")
- Internationalized domain names
Missing documentation that explains the function's purpose and expectations.
Consider adding JSDoc comments and expanding the regex pattern:
+/** + * Validates if a string is a properly formatted domain URL + * @param domain - The domain URL to validate (must include http:// or https://) + * @returns boolean indicating if the domain is valid + */ export const checkDomainWithRegex = (domain: string) => { const regex = - /^(https?:\/\/)[A-Za-z0-9]+([-.][A-Za-z0-9]+)*\.[A-Za-z]{2,}(:[0-9]{1,5})?(\/\S*)?$/; + /^(https?:\/\/)((localhost)|([A-Za-z0-9]+([-.][A-Za-z0-9]+)*\.[A-Za-z0-9]{2,})|((\d{1,3}\.){3}\d{1,3}))(:[0-9]{1,5})?(\/\S*)?$/; return regex.test(domain); };This enhanced regex adds support for localhost, IP addresses, and more flexible TLDs while maintaining the requirement for HTTP/HTTPS schemes.
web-server/pages/api/internal/[org_id]/__tests__/github.test.ts (1)
7-7:
⚠️ Potential issueTypo in the DEFAULT_GH_URL constant
The imported DEFAULT_GH_URL constant has a typo in its definition. It's defined as 'https://api.githb.com' (missing an 'u') according to the relevant code snippet, which would cause API requests to fail.
Make sure to fix the URL in the constants file:
// In web-server/src/constants/urls.ts -export const DEFAULT_GH_URL = 'https://api.githb.com'; +export const DEFAULT_GH_URL = 'https://api.github.com';Committable suggestion skipped: line range outside the PR's diff.
requested changes addressed 👍 |
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.
Actionable comments posted: 1
♻️ Duplicate comments (1)
web-server/pages/api/internal/[org_id]/__tests__/github.test.ts (1)
27-30
: Same mock implementation issue as aboveThis has the same issue with mocking the
then
property directly.🧰 Tools
🪛 Biome (1.9.4)
[error] 29-29: Do not add then to an object.
(lint/suspicious/noThenProperty)
🧹 Nitpick comments (3)
web-server/pages/api/internal/[org_id]/__tests__/github.test.ts (3)
17-20
: Mock implementation needs attentionThe static analysis tool flagged the use of the
then
property in your mock. While this works for the test, it's generally not recommended to mockthen
directly as it can lead to unexpected behavior and makes the test less maintainable.Consider refactoring to use
mockResolvedValue
directly on the query chain:-(db as jest.Mock).mockReturnValue({ - where: jest.fn().mockReturnThis(), - then: jest.fn().mockResolvedValue(mockMeta) -}); +(db as jest.Mock).mockReturnValue({ + where: jest.fn().mockReturnThis(), + mockResolvedValue: jest.fn().mockResolvedValue(mockMeta) +});Alternatively, structure your mock to better reflect Knex's query builder pattern:
+(db as jest.Mock).mockReturnValue({ + where: jest.fn().mockReturnThis(), + then: jest.fn((callback) => Promise.resolve(callback(mockMeta))) +});🧰 Tools
🪛 Biome (1.9.4)
[error] 19-19: Do not add then to an object.
(lint/suspicious/noThenProperty)
45-46
: Fix typo in error messageThere's a typo in the error message: "occured" should be "occurred".
-'Error occured while getting custom domain from database:', +'Error occurred while getting custom domain from database:',
9-12
: Consider adding beforeEach for common test setupYou're resetting mocks after each test, but you may want to consider setting up common mocks in a
beforeEach
block to reduce repetition across test cases.describe('GitHub URL utilities', () => { afterEach(() => { jest.resetAllMocks(); }); + + beforeEach(() => { + // Common setup for all tests can go here + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
web-server/jest.config.js
(1 hunks)web-server/jest.setup.js
(1 hunks)web-server/pages/api/internal/[org_id]/__tests__/github.test.ts
(1 hunks)web-server/pages/api/internal/[org_id]/utils.ts
(4 hunks)web-server/src/constants/urls.ts
(1 hunks)web-server/src/content/Dashboards/ConfigureGithubModalBody.tsx
(7 hunks)web-server/src/content/Dashboards/ConfigureGitlabModalBody.tsx
(1 hunks)web-server/src/utils/__tests__/domainCheck.test.ts
(1 hunks)web-server/src/utils/auth.ts
(3 hunks)web-server/src/utils/domainCheck.ts
(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- web-server/src/utils/tests/domainCheck.test.ts
- web-server/src/constants/urls.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- web-server/src/content/Dashboards/ConfigureGitlabModalBody.tsx
- web-server/jest.setup.js
- web-server/jest.config.js
- web-server/src/utils/domainCheck.ts
- web-server/pages/api/internal/[org_id]/utils.ts
- web-server/src/utils/auth.ts
- web-server/src/content/Dashboards/ConfigureGithubModalBody.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
web-server/pages/api/internal/[org_id]/__tests__/github.test.ts (1)
web-server/src/constants/urls.ts (1)
DEFAULT_GH_URL
(1-1)
🪛 Biome (1.9.4)
web-server/pages/api/internal/[org_id]/__tests__/github.test.ts
[error] 19-19: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 29-29: Do not add then to an object.
(lint/suspicious/noThenProperty)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: All file linting
🔇 Additional comments (3)
web-server/pages/api/internal/[org_id]/__tests__/github.test.ts (3)
1-84
: Test coverage looks good for GitHub Enterprise integrationThe tests cover the key scenarios for working with custom GitHub domains, including error handling and URL normalization. This provides good coverage for the GitHub Enterprise Server integration feature.
🧰 Tools
🪛 Biome (1.9.4)
[error] 19-19: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 29-29: Do not add then to an object.
(lint/suspicious/noThenProperty)
78-82
:✅ Verification successful
Verify GraphQL API URL format for GitHub Enterprise
Similarly, the test expects the custom domain GraphQL URL to be
https://domain/api/graphql
. Confirm this matches the actual GitHub Enterprise GraphQL endpoint structure.
🌐 Web query:
What is the correct GraphQL API URL format for GitHub Enterprise Server?
💡 Result:
The correct GraphQL API URL format for GitHub Enterprise Server is:
https://<HOSTNAME>/api/graphql
Replace
<HOSTNAME>
with the network address or domain of your GitHub Enterprise Server instance. For example, if your GitHub Enterprise Server is available atgithub.company.com
, your GraphQL endpoint would be:https://github.company.com/api/graphql
This endpoint is used for all GraphQL operations—both queries and mutations—and does not change based on the type of data you are requesting[3][5][6].
Citations:
- 1: https://docs.github.com/enterprise-cloud@latest/graphql
- 2: https://docs.github.com/en/[email protected]/graphql/overview/about-the-graphql-api
- 3: https://docs.github.com/en/[email protected]/graphql/guides/forming-calls-with-graphql
- 4: https://docs.github.com/en/[email protected]/admin/overview/about-the-github-enterprise-api
- 5: https://docs.github.com/en/[email protected]/graphql/guides/introduction-to-graphql
- 6: https://docs.github.com/en/enterprise-cloud@latest/graphql/guides/managing-enterprise-accounts
- 7: https://community.zenhub.com/t/what-is-the-format-of-the-graphql-endpoint-with-enterprise/2078
- 8: https://docs.github.com/enterprise-cloud@latest/graphql/overview/about-the-graphql-api
- 9: https://www.cloudbees.com/blog/an-introduction-to-graphql-via-the-github-api
- 10: https://docs.github.com/enterprise-cloud@latest/graphql/guides/using-the-explorer
Verified GraphQL API URL format is correct
The test’s expectation of
https://api.github.local/api/graphql
matches the official GitHub Enterprise Server GraphQL endpoint structure (https://<HOSTNAME>/api/graphql
). No changes required.
58-62
:✅ Verification successful
Verify REST API URL format for GitHub Enterprise
The test expects the custom domain REST API URL to follow the format
https://domain/api/v3/path
. Make sure this matches GitHub Enterprise's actual API URL structure, as some versions or configurations might use different paths.
🌐 Web query:
What is the correct API URL format for GitHub Enterprise Server?
💡 Result:
The correct API URL format for GitHub Enterprise Server (GHES) is:
http(s)://HOSTNAME/api/v3/PATH
- Replace
HOSTNAME
with your GitHub Enterprise Server instance's hostname.- Replace
PATH
with the specific API endpoint you wish to access, such asusers
,repos
, or another resource path[4][9].Examples:
- List all users:
https://ghe.example.com/api/v3/users
- Get repository details:
https://ghe.example.com/api/v3/repos/OWNER/REPO
Notes:
- For most GHES REST API endpoints, this base URL format is used[4][9].
- For some server administration tasks (using the Manage GitHub Enterprise Server API), include the relevant port (typically
8443
for TLS or8080
for non-TLS), for example:
https://ghe.example.com:8443/manage
[2][4].- For regular repository and user endpoints, use the base
/api/v3/
prefix as shown above[4][9].This URL format ensures compatibility with the GHES REST API and should be used in API requests to your enterprise instance.
Citations:
- 1: https://docs.github.com/rest/reference/enterprise-admin
- 2: https://docs.github.com/en/[email protected]/rest/enterprise-admin/manage-ghes
- 3: https://docs.github.com/enterprise-cloud@latest/admin/managing-your-enterprise-account/changing-the-url-for-your-enterprise
- 4: https://docs.github.com/en/[email protected]/rest/enterprise-admin
- 5: https://docs.github.com/rest/repos/repos
- 6: https://docs.github.com/en/[email protected]/admin/overview/about-the-github-enterprise-api
- 7: https://docs.github.com/en/rest/repos/repos?apiVersion=2022-11-28
- 8: https://docs.github.com/en/[email protected]/rest/using-the-rest-api/best-practices-for-using-the-rest-api
- 9: https://docs.github.com/[email protected]/rest/using-the-rest-api/getting-started-with-the-rest-api
- 10: https://docs.github.com/github-ae@latest/rest/users/users
Confirmed correct GitHub Enterprise REST API URL format
The test’s expectation
https://git.sujai.com/api/v3/repos/owner/repo
matches the documented GitHub Enterprise Server API structure (https://HOSTNAME/api/v3/PATH
). No changes needed.
|
||
import { db } from '@/utils/db'; | ||
import * as githubUtils from '../utils'; | ||
import { DEFAULT_GH_URL } from '@/constants/urls'; |
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.
Critical issue with DEFAULT_GH_URL constant
There appears to be a typo in the imported constant from @/constants/urls.ts
. The constant is defined as 'https://api.githb.com'
(missing an 'u'), which will cause all tests and production code to use an incorrect GitHub API URL.
#!/bin/bash
# Verify the constant definition in the urls.ts file
cat web-server/src/constants/urls.ts | grep DEFAULT_GH_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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
web-server/pages/api/internal/[org_id]/__tests__/github.test.ts (1)
45-46
: Minor typo in error message.There's a small typo in the error message: "Error occured while getting" should be "Error occurred while getting".
- 'Error occured while getting custom domain from database:', + 'Error occurred while getting custom domain from database:',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
web-server/jest.config.js
(1 hunks)web-server/jest.setup.js
(1 hunks)web-server/pages/api/internal/[org_id]/__tests__/github.test.ts
(1 hunks)web-server/pages/api/internal/[org_id]/utils.ts
(4 hunks)web-server/src/constants/urls.ts
(1 hunks)web-server/src/content/Dashboards/ConfigureGithubModalBody.tsx
(7 hunks)web-server/src/content/Dashboards/ConfigureGitlabModalBody.tsx
(1 hunks)web-server/src/utils/__tests__/domainCheck.test.ts
(1 hunks)web-server/src/utils/auth.ts
(3 hunks)web-server/src/utils/domainCheck.ts
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- web-server/src/utils/tests/domainCheck.test.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- web-server/jest.setup.js
- web-server/jest.config.js
- web-server/src/content/Dashboards/ConfigureGitlabModalBody.tsx
- web-server/src/constants/urls.ts
- web-server/src/utils/domainCheck.ts
- web-server/src/utils/auth.ts
- web-server/pages/api/internal/[org_id]/utils.ts
- web-server/src/content/Dashboards/ConfigureGithubModalBody.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
web-server/pages/api/internal/[org_id]/__tests__/github.test.ts (1)
web-server/src/constants/urls.ts (1)
DEFAULT_GH_URL
(1-1)
🪛 Biome (1.9.4)
web-server/pages/api/internal/[org_id]/__tests__/github.test.ts
[error] 19-19: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 29-29: Do not add then to an object.
(lint/suspicious/noThenProperty)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: All file linting
🔇 Additional comments (4)
web-server/pages/api/internal/[org_id]/__tests__/github.test.ts (4)
7-7
: Previous issue with DEFAULT_GH_URL has been fixed.The constant is now correctly imported from
@/constants/urls.ts
and based on the provided code snippet, it contains the proper URL 'https://api.github.com'.
14-49
: Well-structured test coverage for the custom domain retrieval.The tests thoroughly cover the key scenarios for
getGitHubCustomDomain
:
- Successfully retrieving a custom domain
- Handling empty results
- Error handling with appropriate logging
This is essential for GitHub Enterprise support and ensures reliable fallback behavior.
Note: The static analysis warnings about "Do not add then to an object" (lines 19, 29) are false positives—this is standard Jest mocking syntax for chained methods.
🧰 Tools
🪛 Biome (1.9.4)
[error] 19-19: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 29-29: Do not add then to an object.
(lint/suspicious/noThenProperty)
51-69
: Good test coverage for REST API URL construction.The tests effectively verify:
- Default URL fallback when no custom domain exists
- Custom domain integration with the correct API path (/api/v3/)
- URL normalization to handle potential path inconsistencies
This ensures robust URL handling for GitHub Enterprise environments.
71-83
: Proper GraphQL endpoint handling for GitHub Enterprise.The tests correctly verify that:
- The default GraphQL endpoint is used when no custom domain is provided
- Custom domains use the appropriate GraphQL path (/api/graphql) for GitHub Enterprise
This completes the URL utilities testing needed for GitHub Enterprise support.
Linked Issue(s)
Fixes #619
Acceptance Criteria fulfillment
Proposed changes (including videos or screenshots)
Further comments
This exactly replicates the custom url implementation for Gitlab
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Tests