Clean-up logic for overriding proxy - #2839
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Overview
This PR removes the workaround for unsetting proxy environment variables from both the TypeScript and JavaScript implementations, as the underlying CodeQL CLI fix makes the workaround unnecessary.
- Removed code that unsets CODEQL_PROXY_* variables in src/analyze-action.ts
- Removed code that unsets CODEQL_PROXY_* variables in lib/analyze-action.js
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/analyze-action.ts | Removed proxy environment variable cleanup as workaround |
| lib/analyze-action.js | Removed proxy environment variable cleanup as workaround |
src/analyze-action.ts:204
- Confirm that the removal of proxy environment variable cleanup does not affect workflows that might still expect them to be unset.
// Unset the CODEQL_PROXY_* environment variables, as they are not needed
lib/analyze-action.js:161
- Verify that the removal of this logic in the JavaScript file does not impact any workflows depending on proxy environment variables.
// Unset the CODEQL_PROXY_* environment variables, as they are not needed
Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more
Sorry, something went wrong.
henrymercer
left a comment
There was a problem hiding this comment.
Makes sense. Can we gate this behind a version check so that we keep the same behaviour for older CLI versions?
Sorry, something went wrong.
| // Unset the CODEQL_PROXY_* environment variables, as they are not needed | ||
| // and can cause issues with older CodeQL CLIs. | ||
| // Check for CODEQL_PROXY_HOST: and if it is empty but set, unset it. | ||
| if (process.env.CODEQL_PROXY_HOST === "" && !(await util.codeQlVersionAtLeast(codeql, "2.20.7"))) { |
There was a problem hiding this comment.
Can you extract this to a constant and place it in feature-flags.ts?
Ideally, we would have pushed this into the CLI itself and added added it to isSupportedToolsFeature, but it's a bit late for that now. Using a hard-coded feature version is best then.
Hmmm...looking through the code, I see that we haven't been to good about moving all hard-coded versions to the feature-flags.ts file. Still, that's something that we've tried to do in the past.
Sorry, something went wrong.
henrymercer
left a comment
There was a problem hiding this comment.
Agree with Andrew's suggestion here. LGTM otherwise.
Sorry, something went wrong.
In #2741, we added this logic due to an issue with the CLI. The CLI has since been fixed and I think it is cleaner to remove this workaround. I am open to hear otherwise and close the PR instead.
Merge / deployment checklist