Add environment variables to signal feature and version to the CLI by aeisenberg · Pull Request #539 · github/codeql-action · GitHub

This PR ensures environment variables are set before any invocation of
the CLI. Here is a list of vars that are set:

https://github.com/github/codeql-coreql-team/issues/1124#issuecomment-852463521

This ensures the CLI knows the features and versions of the containing
actions/runner.

Additionally:

Fix the user agent so that it more closely aligns with user agent
spec
Refactor environment variable initialization so that it all happens in
one place and call.

Note that prepareLocalRunEnvironment() should only be run in actions mode.
And the method is most likely deprecated and
should be removed. I originally added it because I had a way of working
where I would run the action from my local machine to test out changes,
but this was always a little flaky. So, I no longer use this way of
working. I will probably remove it soon.

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.

Are all these options only relevant to the action and not to the runner? Just wondering if they should be in actions-util.ts or somewhere shared.

They should be set by both the action and the runner. (We might need a short word to use about "action and runner both").

Yes, I've confusingly been using codeql-action to signify both the action and the runner. And actions or runner to signify one mode or the other. (And I've probably not been entirely consistent at that.) If you have better terms, please let me know.

If there are no better solutions, I'll just make sure to clarify the meaning of all variables, etc in comments.

Comments are fine by me. My original point was more just that this is in actions-util which was meant to be for code only relating to actions. I'm surprised that when we call setMode/initializeEnvironment from in runner.ts it isn't generating an alert from our custom query.

I think the query might be wrong in fact. I've opened #542

Ahhh...I see what you mean. I was never clear on the distinction between util.ts and actions-utl.ts. I should probably move the mode-related methods to util.ts.

This PR ensures environment variables are set before any invocation of the CLI. Here is a list of vars that are set: github/codeql-core#1124 (comment) This ensures the CLI knows the features and versions of the containing actions/runner. Additionally: - Fix the user agent so that it more closely aligns with user agent spec - Refactor environment variable initialization so that it all happens in one place and call. - Move Mode, getRequiredEnvParam, setMode, getMode out of actions-util and into util. actions-util is meant for utils only called by the action, not the runner. The `prepareLocalRunEnvironment()` method is most likely deprecated and should be removed. I originally added it because I had a way of working where I would run the action from my local machine to test out changes, but this was always a little flaky. So, I no longer use this way of working. I will probably remove it soon.

This is a functionality that never worked perfectly and hasn't been used for a while. This allows developers to run the action on their local machine, but the run was always flaky and never 100% mirrored what was happening on the actions runner.

Oops. A bit of a PR screw-up here. The dependent PR #540 was merged into this one.

I also made changes to migrate getMode, setMode, and the env vars from actions-util into util. Can you all take another look? Probably best to go commit-by-commit for this now.

Must access the env var directly in order to avoid recursive calls to `getRequiredEnvParam`.

Looks reasonable, though definitely worth another look from Robert.

Why do we print this here? It looks like it could be left-over debugging code.

This is just copied from the original method. I can remove in order to simplify things.

Just copied from earlier. I will remove.

Should this stay in the actions-only area? The CODEQL_ACTION_TEMP and RUNNER_TEMP variables it looks for only exist on actions. Confusingly, that RUNNER_TEMP is referring to the process that "runs" the actions, and is not related to our codeql-runner.

Good point. I will move back.

Moved back to the actions-util.ts module.

Generally looks good to me too. I've made a few comments throughout but nothing major. Feel free to merge when you think it's ready.

Also, move `getTemporaryDirectory` back to `actions-util`.

Thanks for the review!

aeisenberg requested review from hmakholm and robertbrignull June 1, 2021 21:50

aeisenberg force-pushed the aeisenberg/env-vars branch from 5658c20 to 539f52a Compare June 1, 2021 21:52

aeisenberg marked this pull request as draft June 1, 2021 21:56

aeisenberg force-pushed the aeisenberg/env-vars branch from 539f52a to 1c59a63 Compare June 1, 2021 22:01

Base automatically changed from aeisenberg/commander to main June 2, 2021 01:35

aeisenberg force-pushed the aeisenberg/env-vars branch from 1c59a63 to 0901686 Compare June 2, 2021 01:39

aeisenberg marked this pull request as ready for review June 2, 2021 01:43

robertbrignull reviewed Jun 2, 2021

View reviewed changes

aeisenberg force-pushed the aeisenberg/env-vars branch from 5fe868c to 3708898 Compare June 2, 2021 18:06

Remove local environment running

2c2ebdc

This is a functionality that never worked perfectly and hasn't been used for a while. This allows developers to run the action on their local machine, but the run was always flaky and never 100% mirrored what was happening on the actions runner.

Merge branch 'main' into aeisenberg/env-vars

082575f

aeisenberg force-pushed the aeisenberg/env-vars branch from 610c3c3 to 082575f Compare June 2, 2021 18:40

Fix Unguarded actions library use code scanning error

cc0733f

Must access the env var directly in order to avoid recursive calls to `getRequiredEnvParam`.

aeisenberg force-pushed the aeisenberg/env-vars branch from e08e072 to cc0733f Compare June 2, 2021 18:55

adityasharad approved these changes Jun 2, 2021

View reviewed changes

Comment thread src/util.ts Outdated

Comment thread src/util.ts Outdated

Comment thread src/util.ts

Comment thread src/util.ts Outdated

robertbrignull reviewed Jun 3, 2021

View reviewed changes

aeisenberg mentioned this pull request Jun 3, 2021

Update unguarded-action-lib.ql to catch uses of actions-util.ts #542

Merged

3 tasks

Cleaning up comments around env vars

f60ef17

Also, move `getTemporaryDirectory` back to `actions-util`.

aeisenberg force-pushed the aeisenberg/env-vars branch from 9acc369 to f60ef17 Compare June 3, 2021 18:33

aeisenberg merged commit cbdf0df into main Jun 3, 2021

aeisenberg deleted the aeisenberg/env-vars branch June 3, 2021 22:35

github-actions Bot mentioned this pull request Jun 7, 2021

Merge main into v1 #549

Merged

5 tasks

+               * Environment variables to be set by the action and possibly used by the
+               * CLI.
+               */
+              enum EnvVar {

                 }
-                core.debug(`${paramName}=${value}`);
+                if (process.env[EnvVar.RUN_MODE] === Mode.actions) {

+                return value;
+              }
+              export function getTemporaryDirectory(): string {

Add environment variables to signal feature and version to the CLI by aeisenberg · Pull Request #539 · github/codeql-action · GitHub

Add environment variables to signal feature and version to the CLI - #539

Add environment variables to signal feature and version to the CLI#539aeisenberg merged 5 commits intomaingithub/codeql-action:mainfrom aeisenberg/env-varsgithub/codeql-action:aeisenberg/env-varsCopy head branch name to clipboard

Conversation

aeisenberg commented Jun 1, 2021

Merge / deployment checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aeisenberg commented Jun 2, 2021

Uh oh!

adityasharad left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertbrignull commented Jun 3, 2021

Uh oh!

aeisenberg commented Jun 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Add environment variables to signal feature and version to the CLI#539
aeisenberg merged 5 commits into
mainfrom
aeisenberg/env-vars