Add octokit throttling behind env var

[jorgectf]

This pull request primarily focuses on enhancing the rate limit handling in the gh-api-client.ts file by integrating the @octokit/plugin-throttling library. It also includes a minor change to the variant-analysis-workflow.yml file to set an environment variable.

Rate limit handling enhancement:

• package.json: Added @octokit/plugin-throttling to the list of dependencies.
• src/gh-api-client.ts: Imported the throttling function from @octokit/plugin-throttling.
• src/gh-api-client.ts: Refactored the getOctokitRequestInterface function to getOctokit and integrated the throttling plugin into the Octokit instance. This new setup will handle rate limits by waiting and retrying requests when limits are reached.
• src/gh-api-client.ts: Updated the updateVariantAnalysisStatus, updateVariantAnalysisStatuses, and getPolicyForRepoArtifact functions to use the new getOctokit function. [1] [2] [3]

Environment variable setting:

• variant-analysis-workflow.yml: Added the CODEQL_VARIANT_ANALYSIS_ACTION_WAIT_ON_RATE_LIMIT environment variable, which enables or disables the rate limit handling based on the MRVA_WAIT_ON_RATE_LIMIT variable.

[robertbrignull]

Adding in this functionality sounds good to me. So long as manual and automated tests pass then I'm happy 👍🏼

[robertbrignull]

Does this correctly handle a "false" value?

I believe process.env.CODEQL_VARIANT_ANALYSIS_ACTION_WAIT_ON_RATE_LIMIT will always be a string value, but the value may be the strings "true" or "false". So then using !! on this will always yield true, because javascript just looks at whether the string is non-empty or not.

That said, do we need this functionality to be gated behind a feature flag or is it something we should enable for all users? I can't see any downsides of it or why a user would want to turn it off. 🤷🏼

[jorgectf]

That's a great catch! IIRC from the testing I did, the absence of the variable repository made enabled: false, but I agree it's not ideal.

do we need this functionality to be gated behind a feature flag

I'm guessing we do, as private repositories would get their Actions usage increased a lot ($$$) in case of a rate limit triggered, but I'm happy to remove the "gate".

[robertbrignull]

the absence of the variable repository made enabled: false, but I agree it's not ideal.

That makes sense, but is the variable ever absent, because we do ${{ (vars.MRVA_WAIT_ON_RATE_LIMIT && true) || false }}? In the false case what value does the env var get inside nodejs?

I'm guessing we do, as private repositories would get their Actions usage increased a lot ($$$) in case of a rate limit triggered, but I'm happy to remove the "gate".

That's a good point it could increase actions usage costs if the workflows sit around forever waiting for the rate limit.

Would it make sense to add a limit so it only retries a bounded number of times? That way it wouldn't increase costs by very much.

Out of interest, who are the intended users right now? (You're welcome to message us on slack or in an internal issue if you don't want to say here on this public PR).

[jorgectf]

That makes sense, but is the variable ever absent, because we do ${{ (vars.MRVA_WAIT_ON_RATE_LIMIT && true) || false }}? In the false case what value does the env var get inside nodejs?

Oops, I just re-tried and it works as you say, so 162ff3b makes the default value a blank string to fix the discrepancy.

Would it make sense to add a limit so it only retries a bounded number of times? That way it wouldn't increase costs by very much.

That would work, but still increase the use a lot and not work for when the rate limit is triggered more than once at a time.

Out of interest, who are the intended users right now? (You're welcome to message us on slack or in an internal issue if you don't want to say here on this public PR).

Me! We are doing heavy use of MRVA lately and this helps me launch queries without worrying about them being canceled.

[charisk]

LGTM