-
Notifications
You must be signed in to change notification settings - Fork 786
Add GitHub Token Support for GraalVM and Refactor Code #849
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: main
Are you sure you want to change the base?
Add GitHub Token Support for GraalVM and Refactor Code #849
Conversation
out of curiosity, I also hit API Rate Limit issues when using |
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.
Pull Request Overview
This pull request adds support for GitHub tokens to address API rate limits and refactors code for improved readability and maintainability.
- Introduces an optional token parameter to the GitHub HTTP headers helper.
- Refactors URL construction and error handling in the GraalVM installer.
- Updates associated tests with better grouping and improved mocking.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
src/util.ts | Added an optional token parameter to getGitHubHttpHeaders to support GitHub token usage. |
src/distributions/graalvm/installer.ts | Refactored file URL construction, error handling, and platform logic for the GraalVM installer. |
tests/distributors/graalvm-installer.test.ts | Improved test grouping, added helpers for mocking HTTP responses, and refined architecture mapping tests. |
Comments suppressed due to low confidence (1)
src/util.ts:186
- Consider updating the function's documentation or inline comments to clearly describe the new optional 'token' parameter and its behavior.
export function getGitHubHttpHeaders(token?: string): OutgoingHttpHeaders {
packageType: 'jdk', | ||
checkLatest: false | ||
}); | ||
|
||
const osType = distribution.getPlatform(); | ||
if (osType === 'windows' && distroArch == 'aarch64') { | ||
if (osType === 'windows' && distroArch === 'aarch64') { | ||
return; // skip, aarch64 is not available for Windows |
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.
[nitpick] Instead of returning early inside the test body to skip the unsupported Windows aarch64 case, consider using a conditional test skip (e.g. test.skip for that iteration) to make the intent clearer.
return; // skip, aarch64 is not available for Windows | |
test.skip('Skipping test: aarch64 is not available for Windows'); |
Copilot uses AI. Check for mistakes.
Description:
Related issue:
#481
Check list: