-
Notifications
You must be signed in to change notification settings - Fork 200
Moving validate logic to Typescript #422
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?
Moving validate logic to Typescript #422
Conversation
7aeebe4 to
8b65045
Compare
41f88d8 to
be6be80
Compare
be6be80 to
f12aa01
Compare
related to google-github-actions#373 related to google-github-actions#375
f12aa01 to
78852e7
Compare
| 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.
The core.summary.write() method returns a Promise. In the warn function, this promise is ignored. If the script finishes execution before the asynchronous write completes, the summary might not be written to the GITHUB_STEP_SUMMARY file and calling .write() multiple times (once per warning) triggers multiple file system operations. Refactor to accumulate all warnings in a list and write to the summary once at the end of execution, ensuring to await the write operation. You will need to make validateInputs async.
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 summary might not be written to the GITHUB_STEP_SUMMARY file
Node js event loop keeps the process alive as long as there are pending asynchronous tasks, so this wont miss.
calling .write() multiple times (once per warning) triggers multiple file system operations
This is intentional for the current scope,
- There are total of 8 lines of file operations happening here and for now and there there won't be any significance performance gain/lose can happen here.
- Prioritizing code simplicity and maintainability for now. While collecting logs into a list is a valid optimization, the current approach serves as a reasonable trade-off until we streamline our overall logging and input validation flow during the upcoming TypeScript migration.
| steps: | ||
| - name: 'Validate Inputs' | ||
| id: 'validate_inputs' | ||
| - name: 'Install pnpm' |
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 adds network and I/O overhead to every workflow using this action, which was previously a near-instantaneous shell script. Consider bundling the TypeScript code into a single JavaScript file and committing it to the repository (e.g., dist/validate_inputs.js). This would allow you to run node dist/validate_inputs.js directly without needing to install dependencies at runtime.
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.
You're right— The dist/*.js approach is good and migration to this is planned soon. Once #373 is resolved, I’ll open a new issue to migrate the composite action in action.yml to a Node.js action (using: 'node20'). This will have the logic to run the js file directly from dist folder and it also streamline input parsing and improve control. I’m targeting this for the v0.18 release. In the meantime, the overhead is minimal; a clean install takes only about 2–3 seconds locally and might take less in CI environments.
computer-name$ npm cache clean -f
npm warn using --force Recommended protections disabled.
computer-name$ rm -rf node_modules/
computer-name$ npm i
> run-gemini-cli@0.1.16 prepare
> husky
added 134 packages, and audited 135 packages in 2s
40 packages are looking for funding
run `npm fund` for details
found 0 vulnerabilities
… `if` condition formatting.
parthasaradhie
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.
`
|
@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. |
|
Above comment is for testing new release |
|
@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. |
Refactor Input Validation Logic
The Validate Inputs step contains extensive conditional logic for validating authentication methods and input combinations. This is moved into a TypeScript module.
Related Issues
#373
#375
Verification
manually verified the new logic by triggering an related action in personal github repo
i set both gemini_api_key and google_api_key which should trigger warning about using both auth tokens at once and it displayed as expected.
run link (expected to fail due to set config): https://github.com/parthasaradhie/hello-world-rpm/actions/runs/20143676300/job/57818088090

warning:
workflow link: release-with-gemini.yaml#L95
workflow config:
