Update unset environment variables PR check by angelapwen · Pull Request #1269 · github/codeql-action · GitHub

The updated Go reconciliation changes surfaced a failing PR check that, upon closer look, was meant to fail previously due to a bug in the CLI in versions < 2.5.1 that didn't propagate environment variables needed by the tracer. This was fixed in v2.5.1.

Therefore this change splits the PR check into two:

checks Java only after version 2.5.1 (ie. not for stable-20210308 which was v2.4.5 or stable-20210319 which was v2.4.6).
checks all other languages for all other CLI versions, as we do now.

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.

This looks good. We could also consider removing support for earlier versions of the CLI — these are quite old now — but I'm happy with this approach.

It's safer to quote all of the bash variables. I did it for cpp, and you should do the same for all the other languages.

Also, it might be nicer to include the expected location in the error message.

Suggested change

      
                    CPP_DB=${{ fromJson(steps.analysis.outputs.db-locations).cpp }}
          
                    CPP_DB="${{ fromJson(steps.analysis.outputs.db-locations).cpp }}"

Have done this!

I'm curious — why does CPP_DB not need the quotes here, but you've added them to its usage later on? Is it because it shouldn't be added while it's being assigned to?

When you are defining a bash variable, you don't need quotes around its name, only its value. When you are using a bash variable, it should always be quoted.

If you don't quote at the appropriate times, whitespace will be considered separate arguments.

Thank you!

Here's an example. Let's say ${{ fromJson(steps.analysis.outputs.db-locations).cpp }} resolves to /path/with a space.

This command:

CPP_DB=${{ fromJson(steps.analysis.outputs.db-locations).cpp }}

will be interpreted as setting the variable CPP_DB/path/with and apply this to a command a with an argument space.

CPP_DB="${{ fromJson(steps.analysis.outputs.db-locations).cpp }}"

Will simply set the CPP_DB variable to /path/with a space. Bash is arcane and learning all these rules takes a long time.

Thanks for the example, that helps! I didn't realize that. Makes sense as to why the variable doesn't need quotes when it's being defined, then.

Suggested change

      
                      echo "Did not create a database for CPP, or created it in the wrong location."
          
                      echo "Did not create a database for CPP, or created it in the wrong location. Expected it in $CPP_DB."

Interesting, I actually read the test differently — I thought expected would be at ${{ runner.temp }}/customDbLocation/* and actual would be $CPP_DB.

Ah...apologies. You might be right here.

Nice!

Hm.. the test is failing with

Did not create a database for CPP, or created it in the wrong location. Expected location was '/home/runner/work/_temp/customDbLocation/*' but actual was '/home/runner/work/_temp/customDbLocation/cpp'

On the bright side, got to test the error message. Do I need the {} operators because of the double quotes, ie. "${CPP_DB}" rather than "CPP_DB"?

Print debugging now

Is there a reason why * is outside of the quotes? I don't think it changes the meaning. Also, does $JAVASCRIPT_DB end in *? I guess so, or else the workflow would be failing.

Yes, it's a wildcard character when outside quotes. @adityasharad and I debugged the reason it failed just now and it was because * was in the quotes and not considered a wildcard 😄

Just looked at this one together -- you need the * outside the quotes so that Bash glob-expands it, otherwise it gets treated as a literal * character.

Bash is awesome. :) Thanks for explaining.

Co-authored-by: Andrew Eisenberg <aeisenberg@github.com>

There were some flaky runner tests that I had to re-run, but it's ready for review again.

Not much we can do about this, but we will need to remember to change this when we no longer support CLI v2.5.9.

Yes, agreed (it's also present in all the other PR checks so we can change them all at once).

So, why exactly do we need to glob here? Is it because we are not sure what the last segement of the directory will be named? And presumably, if there are two entries in the directory, this test will always fail? What if there is a whitespace in this directory segment? I think the test wuld fail, but can this conceivably happen?

So, why exactly do we need to glob here? Is it because we are not sure what the last segement of the directory will be named?

I'm not exactly sure (I didn't write this test originally) — I think we could remove the * and replace it with the name of the directory, which AFAIK is just the language (cpp for example)

And presumably, if there are two entries in the directory, this test will always fail?

I don't think this is true, because currently the ${RUNNER_TEMP}/customDbLocation/ directory should have all the subdirectories named by each language in them.

What if there is a whitespace in this directory segment? I think the test wuld fail, but can this conceivably happen?

Which directory segment do you mean — the very last one that is just the language? I don't think it can happen, but if it did happen, I think it would fail too. Maybe it would just be better to explicitly write out the language rather than use the * at all.

Wild...I didn't know that. Just tried it out. if [[ "$var" == * ]]; will match if any file in the current directory matches $var.

It would definitely be clearer if you replaced * with the language name, but I don't think it's a blocker.

* Only test Java for CLI v2.5+ * Improve bash code style * Set Actions error messages Co-authored-by: Andrew Eisenberg <aeisenberg@github.com>

Only test Java for CLI v2.5+

ab921b6

angelapwen force-pushed the angelapwen/update-unset-env-var-check branch from dc17b28 to ab921b6 Compare September 26, 2022 23:06

angelapwen marked this pull request as ready for review September 26, 2022 23:50

angelapwen requested a review from a team as a code owner September 26, 2022 23:51

henrymercer previously approved these changes Sep 27, 2022

View reviewed changes

Comment thread .github/workflows/unset-environment-old-cli.yml Outdated

Add explanation in PR check comment

81e0392

angelapwen dismissed henrymercer’s stale review via 81e0392 September 27, 2022 17:15

aeisenberg reviewed Sep 27, 2022

View reviewed changes

Comment thread .github/workflows/unset-environment-new-cli.yml Outdated

Comment thread .github/workflows/unset-environment-new-cli.yml Outdated

Improve bash code style

2bdcdfe

angelapwen force-pushed the angelapwen/update-unset-env-var-check branch from 99cf52d to 2bdcdfe Compare September 27, 2022 17:54

Bash syntax fixups

50fd69b

aeisenberg previously approved these changes Sep 27, 2022

View reviewed changes

angelapwen added 2 commits September 27, 2022 11:58

Fix typo

bf48a18

Print variables for debugging

d224372

angelapwen dismissed aeisenberg’s stale review via d224372 September 27, 2022 19:00

angelapwen added 3 commits September 27, 2022 12:15

Fix more bash typos

8374a4e

Fix bash syntax bugs

8024938

Set Actions error messages

231835d

aeisenberg reviewed Sep 27, 2022

View reviewed changes

Comment thread .github/workflows/unset-environment-new-cli.yml Outdated

Add whitespace

e161371

Co-authored-by: Andrew Eisenberg <aeisenberg@github.com>

angelapwen requested a review from aeisenberg September 27, 2022 20:53

angelapwen mentioned this pull request Sep 27, 2022

[DRAFT] Investigate PR Checks when feature flags are on #1262

Closed

3 tasks

aeisenberg reviewed Sep 27, 2022

View reviewed changes

Replace * matching in bash script

9d42ae7

aeisenberg approved these changes Sep 27, 2022

View reviewed changes

angelapwen merged commit 1f0700d into main Sep 27, 2022

angelapwen deleted the angelapwen/update-unset-env-var-check branch September 27, 2022 22:55

angelapwen added a commit that referenced this pull request Sep 28, 2022

Update unset environment variables PR check (#1269 )

cda5896

* Only test Java for CLI v2.5+ * Improve bash code style * Set Actions error messages Co-authored-by: Andrew Eisenberg <aeisenberg@github.com>

This was referenced Sep 29, 2022

Merge main into releases/v2 #1276

Merged

Merge releases/v2 into releases/v1 #1278

Merged

+                      TEST_MODE: true
+                  - shell: bash
+                    run: |
+                      CPP_DB=${{ fromJson(steps.analysis.outputs.db-locations).cpp }}

                       JAVASCRIPT_DB="${{ fromJson(steps.analysis.outputs.db-locations).javascript }}"
-                      if [[ ! -d "$JAVASCRIPT_DB" ]] || [[ ! "$JAVASCRIPT_DB" == "${RUNNER_TEMP}/customDbLocation/*" ]]; then
-                        echo "Did not create a database for Javascript, or created it in the wrong location." \
+                      if [[ ! -d "$JAVASCRIPT_DB" ]] || [[ ! "$JAVASCRIPT_DB" == "${RUNNER_TEMP}/customDbLocation/"* ]]; then

+                    matrix:
+                      include:
+                      - os: ubuntu-latest
+                        version: stable-20210809

+                  - shell: bash
+                    run: |
+                      CPP_DB="${{ fromJson(steps.analysis.outputs.db-locations).cpp }}"
+                      if [[ ! -d "$CPP_DB" ]] || [[ ! "$CPP_DB" == "${RUNNER_TEMP}/customDbLocation/"* ]]; then

	CPP_DB=${{ fromJson(steps.analysis.outputs.db-locations).cpp }}
	CPP_DB="${{ fromJson(steps.analysis.outputs.db-locations).cpp }}"

	echo "Did not create a database for CPP, or created it in the wrong location."
	echo "Did not create a database for CPP, or created it in the wrong location. Expected it in $CPP_DB."

Update unset environment variables PR check by angelapwen · Pull Request #1269 · github/codeql-action · GitHub

Update unset environment variables PR check - #1269

Update unset environment variables PR check#1269angelapwen merged 11 commits intomaingithub/codeql-action:mainfrom angelapwen/update-unset-env-var-checkgithub/codeql-action:angelapwen/update-unset-env-var-checkCopy head branch name to clipboard

Conversation

angelapwen commented Sep 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge / deployment checklist

Uh oh!

henrymercer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aeisenberg left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

Uh oh!

Uh oh!

aeisenberg left a comment

Choose a reason for hiding this comment

Uh oh!

angelapwen commented Sep 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

angelapwen commented Sep 27, 2022

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Update unset environment variables PR check#1269
angelapwen merged 11 commits into
mainfrom
angelapwen/update-unset-env-var-check

angelapwen commented Sep 26, 2022 •
edited
Loading

angelapwen commented Sep 27, 2022 •
edited
Loading