ci: fix CI skip script hole

[zalimeni]

In some environments, the script will not fail despite SKIP_CHECK_BRANCH being unset, leading to the script explicitly skipping CI when it should fail fast.

Meta-comment: we should consider transitioning to paths-ignore. Even though it'd be a bit more copypasta, the upside is we can't silently skip and pass tests + security scans by accident if a future bug or misconfiguration occurs.

Meta-meta-comment: looks like the above is currently infeasible, more on why and alternatives to maintaining our script here.

Description

Example script failure -> skipped CI: https://github.com/hashicorp/consul/actions/runs/10851790913/job/30116333377#step:3:5 (this workflow no longer uses the script, and relies on paths-ignore instead).

Testing

❯ .github/scripts/check_skip_ci.sh; echo $?
.github/scripts/check_skip_ci.sh: line 16: SKIP_CHECK_BRANCH: SKIP_CHECK_BRANCH is required
1

PR Checklist

• updated test coverage
• external facing docs updated
• appropriate backport labels added
• not a security concern

[zalimeni]

This subshell ignores the set -e at the top of the script, so this ended up setting files_to_check to an empty string (it seems). Some envs won't tolerate the later array expansion of that value, but some CI shells seem to allow it, so the failure never causes issues and the script does the "unsafe default" of skipping CI.

[dduzgun-security]

LGTM 👍

[zalimeni]

Just calling out here for posterity (cc @dduzgun-security): I think the reason we aren't able to use paths-ignore generally is that there is no way to make required checks passed if their workflow is skipped by a path filter. Because these particular security scan runs are proactive and not required (unlike others done internally), we can use path filtering in them, but not in integration tests, unit tests, etc.

That said, there are some battle-tested options out there like https://github.com/dorny/paths-filter written in response to this open FR that seem promising, and might relieve us of the burden of maintaining a correct implementation, which is tricky since it's ultimately about always failing the script or always setting skip explicitly to false in the case of a non-skippable file - i.e., the default when the script runs and exits 0 is inherently unsafe.