This test was passing before when we were streaming zstd bundles with a CodeQL Action version that wasn't respecting proxy settings, presumably because the CodeQL Action could still access the bundle without the proxy.

Can we update the test to ensure that we're downloading the bundle via the proxy?

In the latest test added as part of this PR, I'm fairly confident we'redownloading the bundle through the proxy.

The reason it worked before is that itdidn't test the streaming of the bundle, because whenlinked is passed as theversion, theprepare-step action was choosing the.tar.gz bundle by default - as example thelatest run from a PR merged recently.

I am not sure though how it makes that selection between the.tar.gz and the.tar.zst one, because in this PR, the same job downloaded the.tar.zst one. But previously, we didn't go through this code because the same job was opting for the.tar.gz when downloading.

Will look more into that and report back any interesting findings.

Have you tested this fallback? For instance, what happens if we partially extract the CodeQL bundle while streaming, but something goes wrong? Is it worth cleaning up the destination directory in this case or is that handled elsewhere?

Yes, I'm quoting from a previous message of mine that probably got lost because of the volume of messages here:

I'm confident we are good without a flag to manage the risk.

To explain why: because the conflicts introduced by a couple of other PRs were extensive, and I couldn't sort out a clean commit history, I opted to instead just manually recreate the PR on top of main (my code changes are small, so easy to manually replay).

In doing so, I made a mistake: I initialised the HTTP client (the one that respects the proxy settings) and passed it to the headers by accident (instead of the http.get options). What happened ishere, but short story, because of the streaming download now being inside of a fallback try/catch, the failure to download the CodeQL pack with streaming resulted in falling back to downloading it and then extracting it.

I've since fixed this, but this little mistake validated the fallback mechanism working as intended, so I believe that this won't cause an incident like last time - if it fails, worst case scenario is that the mechanism won't work as expected and fall back to the previous mechanism (download and then extract), with the impact being that we lose a few seconds per run (compared to a failed run).

Does this answer your question satisfactorily?

Thank you, that answers the first part of my question. I still wonder if it is worth cleaning up the destination directory as part of thecatch in case we managed to extract some of the CodeQL bundle but not a complete CodeQL bundle.

I agree this is a good idea, and something prudent for us to do before we retry.

Added in4c20d4f.

Was this line added due to a bad merge?

Hm, I will have a look.

This was reported as a conflict in the GitHub UI, but when I did agit merge locally, it had no problem automatically resolving the conflict. Perhaps it did a weird thing because of my.gitconfig.

I cannot find precisely what went wrong here, as the merge log is past my terminal character limit. I remember it saying that it resolved it automatically using theort method, but it apparently went wrong there.

This is now fixed in9e8cd42.

Going forward I will be more careful validating these merges went right.

Don't prefix a warning withError.

But, do we even need the prefix at all?

Maybe reverting this is better:

Thank you for catching this.

Seeing it reported now, I agree it looks a bit silly.

My rationale for changing this was that this was rendering an error that was generated in the process of unpacking as a simple warning, with no other context whatsoever, which could be confusing to a user when interpreting the runner log (example).

I'm still motivated to decorate it somehow, as reverting may not satisfy this concern, but I will think about a cleaner decoration that doesn't look weird/silly.

Okay, this is what I came up with:88bcf64

I've reworded the warnings to flow a bit more naturally for me, and to allow me to embed the error message in an indicative but non-invasive way.

Is this workable for you@aeisenberg ? Or do you have a strong preference for the previous state of affairs?

I think we can simplify this. I can give some suggestions, but not quite right now.

See here for an alternative readme:https://github.com/github/codeql-action/compare/NlightNFotis/detect_use_proxy_when_streaming...aeisenberg/justfile-readme?quick_pull=1#diff-b90d48e45a6813fad7195d74075f05bdf05919291a02116fe8a6b67691b1266a

Thank you for this! I agree it looks cleaner.

Willcherry-pick this in.