workaround: disable streaming when downloading codeql bundle by NlightNFotis · Pull Request #2599 · github/codeql-action (original) (raw)
Description
This PR disables streaming the extraction of the CodeQL bundle as a temporary workaround to the issue described in #2593.
A proper fix will be implemented later so that we can stream while respecting proxy settings.
Fixes #2593
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you reintroduce the CODEQL_ACTION_ZSTD_BUNDLE_STREAMING_EXTRACTION environment variable so the PR check pr-checks/checks/zstd-bundle-streaming.yml will continue to test that streaming works? Otherwise LGTM.
| if (compressionMethod === "zstd" && process.platform === "linux") { |
|---|
| // TODO: Re-enable streaming when we have a more reliable way to respect proxy settings. |
| // eslint-disable-next-line no-constant-condition, no-constant-binary-expression |
| if (false && compressionMethod === "zstd" && process.platform === "linux") { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you turn false into an environment variable? This way at least we can selectively enable it.
Maybe something like:
| if (false && compressionMethod === "zstd" && process.platform === "linux") { |
|---|
| if (process.env[EnvVar.STREAM_BUNDLE_DOWNLOAD] !== "true" && compressionMethod === "zstd" && process.platform === "linux") { |
And then add the variable to the EnvVar class?
@aeisenberg @henrymercer , thank you both for your reviews. I've implemented Henry's request which I found to be in the intersection of what both of you suggested. Would it be possible to have another look at this?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good after you can update the comments to link to the appropriate issue that tracks the TODO.
| let stubConfig: Config; |
| // TODO: Remove when when we no longer need to pass in features |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you link to the issue for this? I think it's this one: #2600
| setupTests(test); |
| // TODO: Remove when when we no longer need to pass in features |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same...can you link to #2600?
| setupTests(test); |
|---|
| // TODO: Remove when when we no longer need to pass in features |
| const expectedFeatureEnablement: FeatureEnablement = initializeFeatures( |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: update initializeFeatures to return FeatureEnablement so we can remove this cast.
@henrymercer @aeisenberg sorry for this, can you please re-approve, because I've added the link that Andrew asked for and it automatically dismissed the reviews.
This was referenced
Nov 12, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})