Skip to content
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

Clean-up logic for overriding proxy #2839

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

marcogario
Copy link
Contributor

@marcogario marcogario commented Apr 1, 2025

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

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.
  • Confirm the changelog has been updated if necessary.

@Copilot Copilot bot review requested due to automatic review settings April 1, 2025 14:53
@marcogario marcogario requested a review from a team as a code owner April 1, 2025 14:53
Copy link
Contributor

@Copilot Copilot AI left a 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 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
Comments suppressed due to low confidence (2)

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

Copy link
Contributor

@henrymercer henrymercer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Can we gate this behind a version check so that we keep the same behaviour for older CLI versions?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants