-
Notifications
You must be signed in to change notification settings - Fork 327
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
workaround: disable streaming when downloading codeql bundle #2599
Conversation
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.
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.
src/tools-download.ts
Outdated
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") { |
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.
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? |
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.
Looks good after you can update the comments to link to the appropriate issue that tracks the TODO.
src/codeql.test.ts
Outdated
@@ -39,6 +41,15 @@ setupTests(test); | |||
|
|||
let stubConfig: Config; | |||
|
|||
// TODO: Remove when when we no longer need to pass in features |
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.
Can you link to the issue for this? I think it's this one: #2600
src/setup-codeql.test.ts
Outdated
@@ -25,6 +27,14 @@ import { | |||
|
|||
setupTests(test); | |||
|
|||
// TODO: Remove when when we no longer need to pass in features |
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.
Same...can you link to #2600?
@@ -25,6 +27,14 @@ import { | |||
|
|||
setupTests(test); | |||
|
|||
// TODO: Remove when when we no longer need to pass in features | |||
const expectedFeatureEnablement: FeatureEnablement = initializeFeatures( |
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.
Nit: update initializeFeatures
to return FeatureEnablement
so we can remove this cast.
427ce46
@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. |
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