Fix Pantheon backup freshness check comparing epoch to formatted date - #97
Fix Pantheon backup freshness check comparing epoch to formatted date#97sgenini wants to merge 2 commits into
Conversation
provider_pull_pantheon_db() reads the last backup's date via
`terminus backup:list ... --field=date`, which returns a raw Unix
epoch timestamp (e.g. 1786491893.1987), and compares it directly
against `$yesterday`, a formatted "Y-m-d H:M:S" string.
Since epoch timestamps for any date after 2001 start with the digit
"1" and formatted "YYYY-..." dates for 2000-2099 start with "2", the
string comparison `"${last_backup}" < "${yesterday}"` is always true
regardless of how recent the backup actually is. This forces a brand
new `terminus backup:create` against the live/production environment
on every single pull, even when a backup from minutes ago already
exists.
The Acquia provider (provider_pull_acquia_db) already converts its
epoch timestamp to the same formatted string before comparing (see
`date --date=@${last_date} ...`); this applies the same pattern here.
Also fixes a secondary bug in the same condition: the "no backup
exists" check compared the literal string "last_backup" instead of
the variable "${last_backup}", so it never actually detected a
missing backup.
|
Warning Review limit reached
Next review available in: 51 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughPantheon database backup handling converts epoch timestamps to the formatted timestamp used for freshness checks. The backup condition now detects a missing timestamp and creates a backup only when no backup exists, the backup is older than one day, or creation is forced. ChangesPantheon backup handling
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pull/pull`:
- Around line 540-542: Update the last_backup conversion logic in pull so it
captures and checks the date command’s exit status before evaluating the backup
condition; handle conversion failures explicitly rather than allowing an empty
value to trigger backup creation. Pass the timestamp to date as the single
quoted argument --date="@${last_backup%%.*}", while preserving the existing
freshness and force checks for successful conversions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| last_backup=$(_exec date --date=@${last_backup%%.*} "+%Y-%m-%d %H:%M:%S") | ||
| fi | ||
| if [[ "${last_backup}" == "" ]] || [[ "${last_backup}" < "${yesterday}" ]] || [[ "$force" == "force" ]]; then |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 '(^|[[:space:]])_exec[[:space:]]*\(|function[[:space:]]+_exec' .Repository: docksal/addons
Length of output: 152
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)pull(/|$)|(^|/)pull$' || true
printf '%s\n' '--- relevant symbols ---'
rg -n -C 5 '_exec|if_failed_error|last_backup|backup:create' . || trueRepository: docksal/addons
Length of output: 22204
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- tracked top-level files ---'
git ls-files | sed -n '1,160p'
printf '%s\n' '--- pull/pull outline and header ---'
wc -l pull/pull
sed -n '1,120p' pull/pull
sed -n '528,552p' pull/pull
printf '%s\n' '--- helper references and definitions ---'
rg -n '(^|[[:space:]])(_exec|if_failed_error|run_cli)[[:space:](=]' --glob '!pull/pull' . || true
rg -n '_exec|if_failed_error' pull/README.md README.md 2>/dev/null || trueRepository: docksal/addons
Length of output: 8913
🌐 Web query:
Docksal fin shell function _exec if_failed_error source
💡 Result:
In the Docksal fin script, if_failed_error is a shell function defined to handle command failures by outputting an error message and exiting the script [1][2][3]. The function is defined as follows: if_failed_error { if [!
Citations:
- 1: https://github.com/docksal/docksal/blob/develop/bin/fin
- 2: https://raw.githubusercontent.com/docksal/docksal/master/bin/fin
- 3: Fin Github interaction: API rate limit exceeded docksal#1790
- 4: Using
fin execbehaves differently and fails when passing existing script as argument docksal#720
🏁 Script executed:
#!/bin/bash
set -euo pipefail
bash <<'BASH'
set -u
_exec() {
printf 'argc=%s\n' "$#"
local i=1
for arg in "$@"; do
printf 'arg[%s]=<%s>\n' "$i" "$arg"
i=$((i + 1))
done
}
last_backup='1700000000.123'
_exec date --date=@${last_backup%%.*} "+%Y-%m-%d %H:%M:%S"
_exec date --date="@${last_backup%%.*}" "+%Y-%m-%d %H:%M:%S"
last_backup='1700000000 1800000000'
_exec date --date=@${last_backup%%.*} "+%Y-%m-%d %H:%M:%S"
_exec date --date="@${last_backup%%.*}" "+%Y-%m-%d %H:%M:%S"
BASHRepository: docksal/addons
Length of output: 491
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp=$(mktemp)
trap 'rm -f "$tmp"' EXIT
curl -fsSL https://raw.githubusercontent.com/docksal/docksal/develop/bin/fin -o "$tmp"
printf '%s\n' '--- helper definitions ---'
rg -n -A 18 -B 3 '(^|[[:space:]])(_exec|if_failed_error)[[:space:](]' "$tmp" || trueRepository: docksal/addons
Length of output: 23433
🏁 Script executed:
#!/bin/bash
set -euo pipefail
bash <<'BASH'
set -u
_exec() {
"$@"
}
if_failed_error() {
local status=$?
printf 'if_failed_error previous_status=%s\n' "$status"
}
run_conversion() {
local last_backup="$1"
last_backup=$(_exec date --date=@${last_backup%%.*} "+%Y-%m-%d %H:%M:%S")
local conversion_status=$?
printf 'input=<%s> status=%s output=<%s>\n' "$1" "$conversion_status" "$last_backup"
if [[ "$last_backup" == "" ]]; then
printf 'backup_branch=entered\n'
else
printf 'backup_branch=not-entered\n'
fi
}
run_conversion 'not-an-epoch'
run_conversion '1700000000.123'
BASHRepository: docksal/addons
Length of output: 336
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp=$(mktemp)
trap 'rm -f "$tmp"' EXIT
curl -fsSL https://raw.githubusercontent.com/docksal/docksal/develop/bin/fin -o "$tmp"
sed -n '5360,5445p' "$tmp"Repository: docksal/addons
Length of output: 3263
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp=$(mktemp)
trap 'rm -f "$tmp"' EXIT
curl -fsSL https://raw.githubusercontent.com/docksal/docksal/develop/bin/fin -o "$tmp"
sed -n '5440,5505p' "$tmp"Repository: docksal/addons
Length of output: 2544
Handle date-conversion failures before creating a backup.
If date fails, last_backup becomes empty and the backup branch runs. Check the conversion status before evaluating freshness. Quote the date argument as --date="@${last_backup%%.*}" to preserve it as one argument.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pull/pull` around lines 540 - 542, Update the last_backup conversion logic in
pull so it captures and checks the date command’s exit status before evaluating
the backup condition; handle conversion failures explicitly rather than allowing
an empty value to trigger backup creation. Pass the timestamp to date as the
single quoted argument --date="@${last_backup%%.*}", while preserving the
existing freshness and force checks for successful conversions.
Addresses CodeRabbit review feedback on docksal#97: pass the timestamp to `date` as a single quoted argument (--date="@${last_backup%%.*}") instead of leaving it unquoted.
1 participant
Fixes #96
Problem
provider_pull_pantheon_db()compares a raw Unix epoch timestamp (fromterminus backup:list --field=date) directly against a formattedY-m-d H:M:Sstring ($yesterday). Since epoch timestamps for any date after 2001 start with1and formatted dates for 2000-2099 start with2, the string comparison is always true — so it force-creates a new backup on the live environment on every pull, never reusing a same-day one.Fix
Convert the epoch timestamp to the same formatted string before comparing, mirroring what
provider_pull_acquia_dbalready does a few hundred lines down in the same file:last_date=$(_exec date --date=@${last_date} "+%Y-%m-%d %H:%M:%S")Also fixes a related typo in the same condition:
[[ "last_backup" == "" ]]was missing the$sigil, so it compared against the literal string"last_backup"instead of the variable, meaning the "no backup exists yet" case never matched either.Testing
Verified against a real Pantheon site:
last_backup=1786491893.1987(raw epoch) vsyesterday=2026-08-10 19:21:58(formatted) →<always true → always creates a new backup.last_backupconverts to2026-08-11 16:44:53, correctly compares as newer thanyesterday, and the existing backup is reused instead of creating a new one.terminus backup:create.Summary by CodeRabbit