-
Notifications
You must be signed in to change notification settings - Fork 355
Add proxy_binary
input to start-proxy
action
#2871
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
This allows a custom binary to be used. Mainly for testing.
Otherwise logs won't get uploaded as artifact, even in debug mode
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 PR introduces a new optional input ("proxy_binary") to the "start-proxy" action so that a custom proxy binary can be specified for testing purposes. Key changes include:
- Adding the "proxy_binary" input definition in start-proxy/action.yml.
- Updating both TypeScript and JavaScript implementations to accept the optional "proxy_binary" input.
- Wrapping process.kill calls in post-action scripts with try-catch blocks for improved error handling.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
start-proxy/action.yml | Added the new optional "proxy_binary" input. |
src/start-proxy-action.ts | Updated proxy binary determination to use the optional input if set. |
src/start-proxy-action-post.ts | Added try-catch block around process.kill for better error logging. |
lib/start-proxy-action.js | Updated proxy binary determination to use the optional input if set. |
lib/start-proxy-action-post.js | Enhanced process.kill error handling with a try-catch block. |
Comments suppressed due to low confidence (2)
src/start-proxy-action.ts:121
- Consider checking that the value returned by getOptionalInput("proxy_binary") is non-empty before defaulting to getProxyBinaryPath, as some implementations might return an empty string when no input is provided.
const proxyBin = actionsUtil.getOptionalInput("proxy_binary") ?? (await getProxyBinaryPath());
lib/start-proxy-action.js:115
- Consider verifying that the result from actionsUtil.getOptionalInput("proxy_binary") is a non-empty string before falling back to getProxyBinaryPath to ensure reliable selection of the proxy binary.
const proxyBin = actionsUtil.getOptionalInput("proxy_binary") ?? (await getProxyBinaryPath());
const proxyBin = | ||
actionsUtil.getOptionalInput("proxy_binary") ?? | ||
(await getProxyBinaryPath()); |
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.
What sort of error is emitted if proxy_binary
doesn't exist or isn't executable? Is there a try-catch somewhere that emits a meaningful error message in this case?
proxy_binary: | ||
description: >- | ||
A path for the proxy binary to use. | ||
required: false |
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 a question, not anything definitive. We're now giving the user an option of specifying an executable. Are there any security vulnerabilities that this opens up? Maybe the answer is "no" since this is already running in an action, which already allows execution of arbitrary code.
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 figured we allow the same sort of thing with the tools
input to the init
Action, so that option is already there. Although I didn't check whether we perform any kind of validation there to verify that we were are running is safe.
This is useful for testing new versions of the proxy before release.
Merge / deployment checklist