-
Notifications
You must be signed in to change notification settings - Fork 0
Ps/ts install #2
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?
Conversation
related to google-github-actions#373 related to google-github-actions#375
… `if` condition formatting.
|
@gemini-cli /review |
|
🤖 Hi @parthasaradhie, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
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 pull request is a great improvement, refactoring the input validation from a shell script to TypeScript. This change significantly enhances maintainability and testability. The introduction of vitest for unit testing is a commendable step towards ensuring code quality.
🔍 General Feedback
- The move to TypeScript for input validation is a solid choice for better type safety and code clarity.
- The unit tests are well-structured and provide good coverage for the new validation logic.
- The addition of a job summary in
action.ymlis a nice touch for better visibility in GitHub Actions.
| .addList([msg]) | ||
| .write(); | ||
| }; | ||
|
|
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.
| .addList([msg]) | |
| .write(); | |
| }; | |
| core.summary | |
| .addHeading('Input validation warnings', 3) | |
| .addList([msg]) | |
| .write(); |
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.
| +export const geminiCliActionsInputs = () => { | |
| + return { | |
| + /** The GCP location to use. */ | |
| + gcpLocation: core.getInput('gcp_location'), | |
| + /** The GCP project ID to use. */ | |
| + gcpProjectId: core.getInput('gcp_project_id'), | |
| + /** The GCP service account to use. */ | |
| + gcpServiceAccount: core.getInput('gcp_service_account'), | |
| + /** The GCP workload identity provider to use. */ | |
| + gcpWorkloadIdentityProvider: core.getInput( | |
| + 'gcp_workload_identity_provider', | |
| + ), | |
| + /** The format of the GCP token to use. */ | |
| + gcpTokenFormat: core.getInput('gcp_token_format'), | |
| + /** The scopes to use for the GCP access token. */ | |
| + gcpAccessTokenScopes: core.getInput('gcp_access_token_scopes'), | |
| + /** The Gemini API key to use. */ | |
| + geminiApiKey: core.getInput('gemini_api_key'), | |
| + /** The version of the Gemini CLI to use. */ | |
| + geminiCliVersion: core.getInput('gemini_cli_version'), | |
| + /** Whether to enable debug mode for the Gemini CLI. */ | |
| + geminiDebug: core.getInput('gemini_debug'), | |
| + /** The Gemini model to use. */ | |
| + geminiModel: core.getInput('gemini_model'), | |
| + /** The Google API key to use. */ | |
| + googleApiKey: core.getInput('google_api_key'), | |
| + /** The prompt to use. */ | |
| + prompt: core.getInput('prompt'), | |
| + /** The settings to use. */ | |
| + settings: core.getInput('settings'), | |
| + /** Whether to use Gemini Code Assist. */ | |
| + useGeminiCodeAssist: getOptionalBooleanInput('use_gemini_code_assist'), | |
| + /** Whether to use Vertex AI. */ | |
| + useVertexAi: getOptionalBooleanInput('use_vertex_ai'), | |
| + /** The extensions to use. */ | |
| + extensions: core.getInput('extensions'), | |
| + /** Whether to upload artifacts. */ | |
| + uploadArtifacts: getOptionalBooleanInput('upload_artifacts'), | |
| + /** Whether to use pnpm. */ | |
| + usePnpm: getOptionalBooleanInput('use_pnpm'), | |
| + /** The name of the workflow. */ | |
| + workflowName: core.getInput('workflow_name'), | |
| + }; | |
| +}; |
|
@gemini-cli /review |
|
🤖 Hi @parthasaradhie, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
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.
📋 Review Summary
This pull request is a great improvement, migrating the input validation from a shell script to a more robust and testable TypeScript implementation. The introduction of Vitest for unit testing is also a welcome addition that will improve the long-term quality of the action.
🔍 General Feedback
- The refactoring of the validation logic is clean and well-tested.
- The new TypeScript files are well-structured.
- The addition of unit tests and coverage reporting significantly improves the project's maintainability.
|
@gemini-cli /review |
|
🤖 Hi @parthasaradhie, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
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.
📋 Review Summary
This Pull Request significantly improves the run-gemini-cli GitHub Action by refactoring input validation logic into TypeScript, introducing Vitest for comprehensive testing, and enhancing the overall development experience. The changes are well-structured and adhere to best practices for GitHub Actions development.
🔍 General Feedback
- The migration of shell-based input validation to TypeScript files greatly enhances maintainability, readability, and testability.
- The addition of Vitest for unit testing, along with detailed test cases and coverage configuration, demonstrates a strong commitment to code quality.
- The new
tsconfig.jsonandvitest.config.tsfiles establish a robust development and testing environment.
| @@ -103,77 +103,35 @@ | |||
| runs: | |||
| using: 'composite' | |||
| steps: | |||
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.
🟢 The refactoring of input validation logic from the action.yml to dedicated TypeScript files (src/validate-inputs.ts and src/inputs.ts) is an excellent improvement. This enhances maintainability, readability, and testability of the action's input handling.
| @@ -5,7 +5,9 @@ | |||
| "scripts": { | |||
| "build": "echo \"No build required for composite action\"", | |||
| "docs": "./node_modules/.bin/actions-gen-readme", | |||
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.
🟢 The addition of test:unit and test:ui scripts to package.json is a good practice for clearly defining different testing workflows within the project. This improves the developer experience and makes it easier to run specific tests.
| @@ -0,0 +1,10 @@ | |||
| { | |||
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 tsconfig.json sets up a solid foundation for TypeScript development within the project. The chosen target, module, and strict options are appropriate for a GitHub Action and promote good coding practices.
| @@ -0,0 +1,30 @@ | |||
| /** | |||
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.
🟢 The vitest.config.ts file provides a comprehensive configuration for unit testing, including JUnit reporting and V8 coverage. This setup significantly improves the testing infrastructure of the project.
| @@ -0,0 +1,60 @@ | |||
| /** | |||
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.
🟢 The src/inputs.ts file correctly extracts and encapsulates the logic for retrieving action inputs. The getOptionalBooleanInput helper is a good addition for safely handling optional boolean inputs.
| @@ -0,0 +1,94 @@ | |||
| import { describe, it, expect, beforeEach } from 'vitest'; | |||
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.
🟢 The tests in src/inputs.test.ts provide good coverage for the geminiCliActionsInputs function, ensuring that all inputs are correctly mapped and handled, including scenarios with unset environment variables. The use of it.each is also a good pattern.
| @@ -0,0 +1,96 @@ | |||
| /** | |||
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.
🟢 The src/validate-inputs.ts file effectively centralizes and validates all action inputs. The use of core.warning and core.summary for reporting validation issues is appropriate and provides clear feedback within the GitHub Actions workflow. The exclusion of the require.main block from coverage is a good practice.
| @@ -0,0 +1,151 @@ | |||
| /** | |||
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.
🟢 The tests in src/validate-inputs.test.ts are comprehensive, covering various scenarios for both valid and invalid input combinations, including error handling. The mocking of @actions/core and the use of expect.stringContaining make these tests robust and effective in ensuring the input validation logic works as expected.
|
@gemini-cli /review |
|
🤖 Hi @parthasaradhie, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
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.
📋 Review Summary
This Pull Request significantly refactors the input validation logic from an inline shell script in action.yml to a dedicated TypeScript module (src/validate-inputs.ts). This change dramatically improves the maintainability, testability, and clarity of the input validation process. The addition of comprehensive unit tests using Vitest, along with a robust testing and coverage configuration, further enhances the quality and reliability of the action.
🔍 General Feedback
- The migration of complex shell scripting to TypeScript is a highly positive change, aligning with best practices for larger, more maintainable codebases.
- The new
src/inputs.tsandsrc/validate-inputs.tsmodules are well-structured, readable, and effectively utilize@actions/core. - The unit tests for both
inputs.tsandvalidate-inputs.tsare thorough, covering various valid and invalid scenarios, and demonstrate excellent use of Vitest's mocking capabilities. - The
tsconfig.jsonandvitest.config.tsfiles provide a solid foundation for TypeScript development and comprehensive testing within the project.
| - name: 'Install pnpm' | ||
| if: |- | ||
| ${{ inputs.use_pnpm == 'true' }} | ||
| uses: 'pnpm/action-setup@41ff72655975bd51cab0327fa583b6e92b6d3061' # ratchet:pnpm/action-setup@v4 |
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.
🟡 It's generally good practice to use an exact version for pnpm/action-setup to ensure deterministic builds across different runs, rather than version: 10 which might refer to 10.x.x. Consider pinning to a specific minor or patch version (e.g., 10.0.0).
| "devDependencies": { | ||
| "@google-github-actions/actions-utils": "^0.8.10", | ||
| "@vitest/coverage-v8": "^4.0.15", | ||
| "@vitest/ui": "^4.0.15", |
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.
🟢 While npx ts-node is convenient for running TypeScript files directly, for production GitHub Actions, it's often more performant to compile TypeScript to JavaScript first and then run the compiled JavaScript. This avoids the overhead of ts-node at runtime. However, for a validation script, the current approach is acceptable.
|
@gemini-cli /review |
|
🤖 Hi @parthasaradhie, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
|
@gemini-cli /review |
|
🤖 Hi @parthasaradhie, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
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.
📋 Review Summary
This pull request is a significant and positive refactoring of the action. Moving the input validation from a bash script to TypeScript improves maintainability and allows for more robust testing. The addition of unit tests with vitest and a job summary are excellent improvements.
🔍 General Feedback
- The migration to TypeScript is well-executed.
- The new tests provide good coverage for the input validation logic.
- The job summary will be very helpful for debugging.
- Consider adding license headers to all new source files for consistency.
- The PR description could be more descriptive of the changes.
| if [[ "${{ inputs.use_pnpm }}" == "true" ]]; then | ||
| pnpm install --silent --no-audit --prefer-offline | ||
| else | ||
| npm ci --silent --no-audit | ||
| fi |
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.
🟢 The if condition for checking inputs.use_pnpm can be simplified by moving it inside the run block. This makes the YAML a bit cleaner.
| if [[ "${{ inputs.use_pnpm }}" == "true" ]]; then | |
| pnpm install --silent --no-audit --prefer-offline | |
| else | |
| npm ci --silent --no-audit | |
| fi | |
| run: | | |
| if [[ "${{ inputs.use_pnpm }}" == "true" ]]; then | |
| pnpm install --silent --no-audit --prefer-offline | |
| else | |
| npm ci --silent --no-audit | |
| fi |
| core.setFailed(`Input validation failed: ${error}`); | ||
| } | ||
| }; | ||
|
|
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.
🟢 The license header in the newly created file src/validate-inputs.ts is a good practice. To maintain consistency across the codebase, it would be beneficial to include similar license headers in the other new files as well.
| /** | |
| * @license | |
| * Copyright 2025 Google LLC | |
| * SPDX-License-Identifier: Apache-2.0 | |
| */ |
|
|
||
| /* v8 ignore if -- @preserve */ | ||
| if (require.main === module) { | ||
| validateInputs(); |
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.
🟢 Adding a JSDoc comment to the validateInputs function would improve code readability and maintainability.
| validateInputs(); | |
| /** | |
| * Validates the inputs for the Gemini CLI action. | |
| */ |
|
@gemini-cli /review |
|
🤖 Hi @parthasaradhie, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
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.
📋 Review Summary
This is an excellent pull request that significantly improves the maintainability and reliability of the action by migrating the input validation logic from a shell script to a well-tested TypeScript implementation.
🔍 General Feedback
- The introduction of unit tests with
vitestis a fantastic addition and sets a great standard for the project. - The code is clean, well-structured, and the changes in
action.ymlare logical and clean. - The only minor issue I found is the copyright year in the new files is set to 2025.
Great job!
| @@ -0,0 +1,96 @@ | |||
| /** | |||
| * @license | |||
| * Copyright 2025 Google LLC | |||
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.
🟢 The copyright year is set to 2025. It should be the current year.
| * Copyright 2025 Google LLC | |
| * Copyright 2024 Google LLC |
| @@ -0,0 +1,151 @@ | |||
| /** | |||
| * @license | |||
| * Copyright 2025 Google LLC | |||
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.
🟢 The copyright year is set to 2025. It should be the current year.
| * Copyright 2025 Google LLC | |
| * Copyright 2024 Google LLC |
| @@ -0,0 +1,60 @@ | |||
| /** | |||
| * @license | |||
| * Copyright 2025 Google LLC | |||
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.
🟢 The copyright year is set to 2025. It should be the current year.
| * Copyright 2025 Google LLC | |
| * Copyright 2024 Google LLC |
| @@ -0,0 +1,30 @@ | |||
| /** | |||
| * @license | |||
| * Copyright 2025 Google LLC | |||
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.
🟢 The copyright year is set to 2025. It should be the current year.
| * Copyright 2025 Google LLC | |
| * Copyright 2024 Google LLC |
hjgdjhfgsd
fasdfkjhdsjfh
asdfjkdshkjfjds
asdfjsdhfjkdsh