refactor(ci): enforce required env variables in build scripts#35990
refactor(ci): enforce required env variables in build scripts#35990
Conversation
Add mandatory environment variable checks to key scripts for improved reliability and early failure detection.
There was a problem hiding this comment.
Pull request overview
This PR refactors environment variable validation in Buildkite CI build scripts to use a uniform shell parameter expansion pattern (: "${VAR:?message}"). This replaces explicit if-else checks with a more concise and consistent approach that provides descriptive error messages when required variables are missing.
Changes:
- Replaced manual if-else validation checks with shell parameter expansion in
upload-test-results.sh - Added environment variable validation to 9 build scripts using the uniform pattern
- Improved error messages with descriptive context for each variable
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
.buildkite/upload-test-results.sh |
Replaced if-else checks for JAVA_TEST_TOKEN and CPP_TEST_TOKEN with parameter expansion validation |
.buildkite/quick-start-guide.sh |
Added validation for SOURCE_DIR |
.buildkite/java.sh |
Added validation for SOURCE_DIR, VESPA_MAVEN_TARGET, and NUM_MVN_THREADS |
.buildkite/install.sh |
Added validation for NUM_CPU_LIMIT and WORKDIR |
.buildkite/go.sh |
Added validation for SOURCE_DIR and WORKDIR |
.buildkite/cpp.sh |
Added validation for SOURCE_DIR and NUM_CPP_THREADS |
.buildkite/cpp-test.sh |
Added validation for NUM_CPU_LIMIT and LOG_DIR |
.buildkite/build-container.sh |
Added validation for VESPA_BUILDOS_LABEL |
.buildkite/bootstrap.sh |
Added validation for SOURCE_DIR and VESPA_CPP_TEST_JARS |
.buildkite/bootstrap-cmake.sh |
Added validation for SOURCE_DIR |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| exit 1 | ||
| fi | ||
| : "${JAVA_TEST_TOKEN:?Environment variable JAVA_TEST_TOKEN must be set (token for uploading Java test results)}" | ||
| : "${CPP_TEST_TOKEN:?Environment variable CPP_TEST_TOKEN must be set (token for uploading C++ test results)}" |
There was a problem hiding this comment.
The script uses WORKDIR (line 53) and LOG_DIR (line 61) but does not validate these required environment variables. For consistency with the pattern established in this PR and other scripts (e.g., install.sh, cpp-test.sh, go.sh), these variables should be validated at the beginning of the script using the shell parameter expansion pattern.
| : "${CPP_TEST_TOKEN:?Environment variable CPP_TEST_TOKEN must be set (token for uploading C++ test results)}" | |
| : "${CPP_TEST_TOKEN:?Environment variable CPP_TEST_TOKEN must be set (token for uploading C++ test results)}" | |
| : "${WORKDIR:?Environment variable WORKDIR must be set (working directory for test results)}" | |
| : "${LOG_DIR:?Environment variable LOG_DIR must be set (directory containing C++ test results)}" |
| if [[ -n "${DEBUG:-}" ]]; then | ||
| set -o xtrace | ||
| fi | ||
|
|
||
| if [[ $BUILDKITE != true ]]; then | ||
| echo "Skipping artifact publishing when not executed by Buildkite." | ||
| exit 0 | ||
| fi | ||
|
|
||
| echo "--- 📊 Uploading test results to Buildkite Analytics" | ||
| if [[ $(arch) == x86_64 ]]; then | ||
| JAVA_TEST_TOKEN=$UNIT_TEST_JAVA_AMD64_TOKEN | ||
| CPP_TEST_TOKEN=$UNIT_TEST_CPP_AMD64_TOKEN | ||
| else | ||
| JAVA_TEST_TOKEN=$UNIT_TEST_JAVA_ARM64_TOKEN | ||
| CPP_TEST_TOKEN=$UNIT_TEST_CPP_ARM64_TOKEN | ||
| fi | ||
|
|
||
| if [[ -z $JAVA_TEST_TOKEN ]]; then | ||
| echo "Missing JAVA_TEST_TOKEN. Exiting." | ||
| exit 1 | ||
| fi | ||
| if [[ -z $CPP_TEST_TOKEN ]]; then | ||
| echo "Missing CPP_TEST_TOKEN. Exiting." | ||
| exit 1 | ||
| fi | ||
| : "${JAVA_TEST_TOKEN:?Environment variable JAVA_TEST_TOKEN must be set (token for uploading Java test results)}" | ||
| : "${CPP_TEST_TOKEN:?Environment variable CPP_TEST_TOKEN must be set (token for uploading C++ test results)}" | ||
|
|
||
| upload_result() { | ||
| curl \ | ||
| -X POST \ | ||
| -H "Authorization: Token token=\"$BUILDKITE_ANALYTICS_TOKEN\"" \ | ||
| -F "data=@$1" \ |
There was a problem hiding this comment.
Using set -o xtrace in this script while handling JAVA_TEST_TOKEN, CPP_TEST_TOKEN and BUILDKITE_ANALYTICS_TOKEN causes their secret values to be echoed in Buildkite logs (for example when evaluating : "${JAVA_TEST_TOKEN:?...}" and running the curl command). Anyone with access to CI logs could reuse these tokens to upload or tamper with test result data via the Buildkite Analytics API. To avoid leaking secrets, ensure xtrace is disabled around code that uses these variables or otherwise prevent commands involving these tokens from being printed to the logs.
| set -o xtrace | ||
| fi | ||
|
|
||
| : "${VESPA_BUILDOS_LABEL:?Environment variable VESPA_BUILDOS_LABEL must be set (build OS label)}" |
There was a problem hiding this comment.
It's not only verified to be set here, but also by set -o nounset when referenced on line 21 below, and by the fallback on lines 30-32.
This is one of the downsides of nounset - it would be better to reach line 31 to print a proper error message, than an unbound error on line 21.
| set -o xtrace | ||
| fi | ||
|
|
||
| : "${SOURCE_DIR:?Environment variable SOURCE_DIR must be set (path to source code)}" |
There was a problem hiding this comment.
Verify that $SOURCE_DIR is a directory is a better test? (Such a test would be even better before nounset)
What
Why