ci: update rotate-keys for additional key#16269
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the key rotation script rotate-keys.sh to support multiple service accounts, specifically adding the observability service account. The feedback recommends assigning command substitution outputs to variables to prevent swallowing errors under set -e, and refactoring the space-separated file types into a Bash array to avoid unquoted variable expansion issues.
| "--filter=CREATED_AT<-p90d" | ||
| "--format=value(KEY_ID)" | ||
| ) | ||
| for old_key in $(gcloud iam service-accounts keys list "${args[@]}"); do |
There was a problem hiding this comment.
In Bash, a failure in a command substitution inside a for loop list (e.g., for x in $(command)) does not trigger set -e, which means if gcloud fails, the script will silently continue without deleting old keys. This violates the Demand Explosive Correctness principle in the repository style guide.
To fix this, assign the command output to a variable first so that set -e can properly catch any failures.
| for old_key in $(gcloud iam service-accounts keys list "${args[@]}"); do | |
| old_keys=$(gcloud iam service-accounts keys list "${args[@]}") | |
| for old_key in ${old_keys}; do |
References
- Demand Explosive Correctness: Never swallow errors or ignore Status types. Fail loudly and explicitly when appropriate. (link)
| fi | ||
|
|
||
| for entry in "${accounts_config[@]}"; do | ||
| IFS="|" read -r sa_email prefix filetypes <<<"${entry}" |
There was a problem hiding this comment.
To avoid unquoted variable expansion (which triggers ShellCheck warning SC2086) and ensure robust word splitting, consider reading the space-separated filetypes into a Bash array.
| IFS="|" read -r sa_email prefix filetypes <<<"${entry}" | |
| IFS="|" read -r sa_email prefix filetypes_str <<<"${entry}" | |
| read -r -a filetypes <<<"${filetypes_str}" |
| "${prefix}-$(date +"%Y-%m" --date="now + 2 weeks")" | ||
| ) | ||
| for key_base in "${active_key_bases[@]}"; do | ||
| for filetype in ${filetypes}; do |
There was a problem hiding this comment.
Use the filetypes array with proper quoting to prevent word splitting and globbing issues.
| for filetype in ${filetypes}; do | |
| for filetype in "${filetypes[@]}"; do |
| done | ||
| io::log_h2 "Checking for stale keyfiles (${prefix})" | ||
| stale_key_base="${prefix}-$(date +"%Y-%m" --date="now - 45 days")" | ||
| for filetype in ${filetypes}; do |
There was a problem hiding this comment.
Use the filetypes array with proper quoting here as well.
| for filetype in ${filetypes}; do | |
| for filetype in "${filetypes[@]}"; do |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #16269 +/- ##
==========================================
- Coverage 92.27% 92.27% -0.01%
==========================================
Files 2218 2218
Lines 206654 206654
==========================================
- Hits 190699 190687 -12
- Misses 15955 15967 +12 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
No description provided.