[#1894] Allow to download a fresh DB using ahoy download-db --no-cache.
#1896
Conversation
WalkthroughThe changes introduce a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Ahoy CLI
participant download-db.sh
participant download-db-lagoon.sh
User->>Ahoy CLI: ahoy download-db [--no-cache]
Ahoy CLI->>Ahoy CLI: Check for --no-cache
alt --no-cache present
Ahoy CLI->>download-db.sh: Set VORTEX_DB_DOWNLOAD_NO_CACHE=1
else --no-cache absent
Ahoy CLI->>download-db.sh: Run normally
end
download-db.sh->>download-db-lagoon.sh: Call script
download-db-lagoon.sh->>download-db-lagoon.sh: Check VORTEX_DB_DOWNLOAD_NO_CACHE
alt VORTEX_DB_DOWNLOAD_NO_CACHE=1
download-db-lagoon.sh->>download-db-lagoon.sh: Force fresh database dump
else
download-db-lagoon.sh->>download-db-lagoon.sh: Use cached dump if available
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
AlexSkrypnyk
force-pushed
the
feature/allow-download-fresh-db
branch
from
706fdd6 to
8922e79
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
scripts/vortex/download-db-lagoon.sh (1)
123-129: Fix remote shell portability: use single ‘=’ in testSSH executes the command via the remote’s default shell, often /bin/sh (dash/ash), where ‘==’ is non-POSIX. Use ‘=’ to avoid false negatives.
Apply this diff:
- "if [ ! -f \"${VORTEX_DB_DOWNLOAD_LAGOON_REMOTE_DIR}/${VORTEX_DB_DOWNLOAD_LAGOON_REMOTE_FILE}\" ] || [ \"${VORTEX_DB_DOWNLOAD_NO_CACHE}\" == \"1\" ] ; then \ + "if [ ! -f \"${VORTEX_DB_DOWNLOAD_LAGOON_REMOTE_DIR}/${VORTEX_DB_DOWNLOAD_LAGOON_REMOTE_FILE}\" ] || [ \"${VORTEX_DB_DOWNLOAD_NO_CACHE}\" = \"1\" ] ; then \Optional hardening (timezone-safe caching): compute the date on the remote rather than locally to ensure “same day” semantics across environments:
# Example: define remote file name inside the remote command remote_file="db_$(date +%Y%m%d).sql" # Then use ${remote_file} instead of expanding a locally computed date.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (3)
.vortex/installer/tests/Fixtures/install/_baseline/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/provision_profile/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/theme_absent/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**
📒 Files selected for processing (5)
.ahoy.yml(1 hunks).vortex/docs/content/workflows/development.mdx(1 hunks).vortex/docs/content/workflows/variables.mdx(1 hunks).vortex/tests/bats/unit/download-db-lagoon.bats(4 hunks)scripts/vortex/download-db-lagoon.sh(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: build (1)
- GitHub Check: build (0)
- GitHub Check: vortex-test-deployment (1)
- GitHub Check: vortex-test-workflow (3)
- GitHub Check: vortex-test-workflow (1)
- GitHub Check: vortex-test-deployment (0)
- GitHub Check: vortex-test-workflow (0)
- GitHub Check: vortex-test-workflow (2)
- GitHub Check: vortex-test-common
- GitHub Check: vortex-test-installer (8.3)
- GitHub Check: vortex-test-installer (8.2)
- GitHub Check: vortex-test-installer (8.4)
🔇 Additional comments (3)
.vortex/tests/bats/unit/download-db-lagoon.bats (1)
146-146: LGTM: correctly enabling the no-cache path for this testSetting VORTEX_DB_DOWNLOAD_NO_CACHE="1" here is correct and matches the scenario.
scripts/vortex/download-db-lagoon.sh (2)
26-26: LGTM: new no-cache flag variableCorrectly exposes VORTEX_DB_DOWNLOAD_NO_CACHE for controlling fresh dump behavior.
104-107: LGTM: safe creation of dump directory with clear loggingIdempotent mkdir -p with a task log improves UX and avoids errors when the directory is missing.
| usage: Download database. Run with "--no-cache" option to force fresh database backup. | ||
| aliases: ['fetch-db'] | ||
| cmd: ./scripts/vortex/download-db.sh | ||
| cmd: | | ||
| case " $* " in *" --no-cache "*) export VORTEX_DB_DOWNLOAD_NO_CACHE=1;; esac | ||
| ./scripts/vortex/download-db.sh | ||
| #;> !PROVISION_TYPE_PROFILE |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Good flag wiring; also forward arguments to the script
The --no-cache detection and env var export look good. Forwarding "$@" to the script improves extensibility if more flags are added later.
Apply this diff:
cmd: |
case " $* " in *" --no-cache "*) export VORTEX_DB_DOWNLOAD_NO_CACHE=1;; esac
- ./scripts/vortex/download-db.sh
+ ./scripts/vortex/download-db.sh "$@"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| usage: Download database. Run with "--no-cache" option to force fresh database backup. | |
| aliases: ['fetch-db'] | |
| cmd: ./scripts/vortex/download-db.sh | |
| cmd: | | |
| case " $* " in *" --no-cache "*) export VORTEX_DB_DOWNLOAD_NO_CACHE=1;; esac | |
| ./scripts/vortex/download-db.sh | |
| #;> !PROVISION_TYPE_PROFILE | |
| usage: Download database. Run with "--no-cache" option to force fresh database backup. | |
| aliases: ['fetch-db'] | |
| cmd: | | |
| case " $* " in *" --no-cache "*) export VORTEX_DB_DOWNLOAD_NO_CACHE=1;; esac | |
| ./scripts/vortex/download-db.sh "$@" | |
| #;> !PROVISION_TYPE_PROFILE |
🤖 Prompt for AI Agents
In the .ahoy.yml file around lines 123 to 128, the current command detects the
--no-cache flag and sets an environment variable but does not forward any
additional arguments to the script. Modify the cmd section to forward all
received arguments ("$@") to the ./scripts/vortex/download-db.sh script to
support future extensibility with more flags.
| dump from the production environment into a `.data` directory. If there was a | ||
| download attempt on the same day (locally or from CI), it will download the | ||
| cached database dump. Use `ahoy fetch-db --no-cache` to create a new database | ||
| dump regardless of the cache. |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Clearer caching behavior; consider noting timezone edge-case or aligning script
The explanation is good. One nuance: the dump filename uses a date suffix; currently the script computes that date locally, which can diverge from CI/remote timezones around day boundaries. Either:
- add a brief note that “same day” is based on the machine running the command, or
- (preferred) adjust the script to compute the date on the remote to avoid drift (see my script comment).
🤖 Prompt for AI Agents
In .vortex/docs/content/workflows/development.mdx around lines 48 to 51, clarify
the caching behavior by adding a note that the "same day" determination for the
dump filename is based on the local machine's timezone, which may differ from CI
or remote environments. Alternatively, update the script to compute the date
suffix on the remote environment to ensure consistent date calculation across
different timezones and avoid cache mismatches.
| ### `VORTEX_DB_DOWNLOAD_NO_CACHE` | ||
|
|
||
| Proceed with download. | ||
| Flag to download a fresh copy of the database. | ||
|
|
||
| Default value: `1` | ||
| Default value: `UNDEFINED` | ||
|
|
||
| Defined in: `scripts/vortex/download-db.sh` | ||
| Defined in: `scripts/vortex/download-db-lagoon.sh` | ||
|
|
||
| ### `VORTEX_DB_DOWNLOAD_REFRESH` | ||
| ### `VORTEX_DB_DOWNLOAD_PROCEED` | ||
|
|
||
| Flag to download a fresh copy of the database. | ||
| Proceed with download. | ||
|
|
||
| Default value: `UNDEFINED` | ||
| Default value: `1` | ||
|
|
||
| Defined in: `scripts/vortex/download-db-lagoon.sh` | ||
| Defined in: `scripts/vortex/download-db.sh` | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Rename looks consistent; add a brief usage note and verify no stale refs
Good swap: NO_CACHE now controls fresh dump creation, PROCEED covers the generic gating. Consider adding that NO_CACHE should be set to "1" to activate.
Run this to ensure no leftover references:
🏁 Script executed:
#!/bin/bash
# Find stale uses of old variable name
rg -n --color=never 'VORTEX_DB_DOWNLOAD_REFRESH'Length of output: 48
No stale references found; please add NO_CACHE usage note
I ran rg -n 'VORTEX_DB_DOWNLOAD_REFRESH' across the repo and found no leftover occurrences. The renaming is consistent with the scripts. Before merging, please update the documentation to clarify how to activate the new flag:
- File:
.vortex/docs/content/workflows/variables.mdx - Location: around lines 669–684 under
VORTEX_DB_DOWNLOAD_NO_CACHE
Suggested diff:
### `VORTEX_DB_DOWNLOAD_NO_CACHE`
Flag to download a fresh copy of the database.
+
+Activate by setting to `1`.
Default value: `UNDEFINED`
Defined in: `scripts/vortex/download-db-lagoon.sh`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ### `VORTEX_DB_DOWNLOAD_NO_CACHE` | |
| Proceed with download. | |
| Flag to download a fresh copy of the database. | |
| Default value: `1` | |
| Default value: `UNDEFINED` | |
| Defined in: `scripts/vortex/download-db.sh` | |
| Defined in: `scripts/vortex/download-db-lagoon.sh` | |
| ### `VORTEX_DB_DOWNLOAD_REFRESH` | |
| ### `VORTEX_DB_DOWNLOAD_PROCEED` | |
| Flag to download a fresh copy of the database. | |
| Proceed with download. | |
| Default value: `UNDEFINED` | |
| Default value: `1` | |
| Defined in: `scripts/vortex/download-db-lagoon.sh` | |
| Defined in: `scripts/vortex/download-db.sh` | |
| ### `VORTEX_DB_DOWNLOAD_NO_CACHE` | |
| Flag to download a fresh copy of the database. | |
| Activate by setting to `1`. | |
| Default value: `UNDEFINED` | |
| Defined in: `scripts/vortex/download-db-lagoon.sh` | |
| ### `VORTEX_DB_DOWNLOAD_PROCEED` | |
| Proceed with download. | |
| Default value: `1` | |
| Defined in: `scripts/vortex/download-db.sh` |
🤖 Prompt for AI Agents
In .vortex/docs/content/workflows/variables.mdx around lines 669 to 684, the
documentation for VORTEX_DB_DOWNLOAD_NO_CACHE is missing a usage note explaining
how to activate this flag. Add a clear explanation under the
VORTEX_DB_DOWNLOAD_NO_CACHE section describing how to set or use this flag to
download a fresh copy of the database, ensuring users understand its purpose and
activation method.
| "- Database dump refresh requested. Will create a new dump." | ||
|
|
There was a problem hiding this comment.
Test expectation does not match implementation; remove the refresh note here
The script prints “Database dump refresh requested. Will create a new dump.” only when VORTEX_DB_DOWNLOAD_NO_CACHE="1". This test doesn’t set it, so this assertion will fail. Remove these lines (or set the env var if the test is meant to cover the no-cache path).
Apply this diff to drop the incorrect expectation:
- "- Database dump refresh requested. Will create a new dump."📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "- Database dump refresh requested. Will create a new dump." |
🤖 Prompt for AI Agents
In .vortex/tests/bats/unit/download-db-lagoon.bats at lines 24 to 25, the test
expects the output "Database dump refresh requested. Will create a new dump."
which only appears when the environment variable VORTEX_DB_DOWNLOAD_NO_CACHE is
set to "1". Since this test does not set that variable, remove the assertion
checking for this output line to fix the mismatch between test expectation and
implementation.
There was a problem hiding this comment.
- is the negative assertion!
There was a problem hiding this comment.
@AlexSkrypnyk You're right—thanks for the clarification. The leading "- " denotes a negative assertion in the run_steps harness, so the tests are correct as written. I retract my earlier suggestion; no change needed here. I've recorded this convention to avoid future confusion.
✏️ Learnings added
Learnt from: AlexSkrypnyk
PR: drevops/vortex#1896
File: .vortex/tests/bats/unit/download-db-lagoon.bats:24-25
Timestamp: 2025-08-08T12:02:24.633Z
Learning: In .vortex/tests/bats/unit Bats tests using ../_helper.bash (run_steps), prefixing a STEPS entry with "- " denotes a negative assertion (the substring must NOT appear in output). Unprefixed entries are positive assertions. Example: "- Database dump refresh requested. Will create a new dump." asserts absence; "Database dump refresh requested. Will create a new dump." asserts presence.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: AlexSkrypnyk
PR: drevops/vortex#0
File: :0-0
Timestamp: 2025-05-29T12:15:32.188Z
Learning: Do not review files in `.vortex/installer/tests/Fixtures/install` directory as they are test fixtures.
Learnt from: AlexSkrypnyk
PR: drevops/vortex#1697
File: .vortex/installer/tests/Fixtures/install/provision_database_lagoon/docs/onboarding.md:16-17
Timestamp: 2025-06-02T00:41:36.321Z
Learning: DO NOT EVER REVIEW FILES IN .vortex/installer/tests/Fixtures/ directory - these are test fixtures and should not be reviewed under any circumstances.
| "- Database dump refresh requested. Will create a new dump." | ||
|
|
There was a problem hiding this comment.
Same mismatch as above; drop the refresh note from this test
This test also doesn’t set VORTEX_DB_DOWNLOAD_NO_CACHE, so expecting the refresh note is incorrect.
Apply this diff:
- "- Database dump refresh requested. Will create a new dump."📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "- Database dump refresh requested. Will create a new dump." |
🤖 Prompt for AI Agents
In .vortex/tests/bats/unit/download-db-lagoon.bats around lines 78 to 79, remove
the log message "- Database dump refresh requested. Will create a new dump."
from the test output expectations because this test does not set
VORTEX_DB_DOWNLOAD_NO_CACHE, so it should not expect a refresh note. Adjust the
test assertions accordingly to exclude this message.
| "Database dump refresh requested. Will create a new dump." | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM: refresh note asserted where NO_CACHE=1 is set
This assertion aligns with the script’s behavior for the no-cache path. Consider using the same message style across tests (no leading dash) for consistency.
🤖 Prompt for AI Agents
In .vortex/tests/bats/unit/download-db-lagoon.bats around lines 131 to 132, the
refresh note message includes a leading dash which is inconsistent with other
tests. Remove the leading dash from the message to match the style used in other
assertions, ensuring consistent message formatting across all tests.
| if [ "$VORTEX_DB_DOWNLOAD_NO_CACHE" == "1" ]; then | ||
| note "Database dump refresh requested. Will create a new dump." | ||
| fi |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Prefer POSIX ‘=’ over ‘==’ in [ test ] for portability
Although the script runs under bash locally, using ‘=’ avoids portability issues and keeps style consistent with the remote change below.
Apply this diff:
-if [ "$VORTEX_DB_DOWNLOAD_NO_CACHE" == "1" ]; then
+if [ "$VORTEX_DB_DOWNLOAD_NO_CACHE" = "1" ]; then
note "Database dump refresh requested. Will create a new dump."
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [ "$VORTEX_DB_DOWNLOAD_NO_CACHE" == "1" ]; then | |
| note "Database dump refresh requested. Will create a new dump." | |
| fi | |
| if [ "$VORTEX_DB_DOWNLOAD_NO_CACHE" = "1" ]; then | |
| note "Database dump refresh requested. Will create a new dump." | |
| fi |
🤖 Prompt for AI Agents
In scripts/vortex/download-db-lagoon.sh around lines 109 to 111, the test
condition uses '==' which is not POSIX compliant and may cause portability
issues. Replace '==' with '=' in the if statement to ensure POSIX compatibility
and maintain consistent style.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1896 +/- ##
===========================================
- Coverage 78.70% 73.25% -5.45%
===========================================
Files 85 39 -46
Lines 5019 3167 -1852
Branches 35 0 -35
===========================================
- Hits 3950 2320 -1630
+ Misses 1069 847 -222 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Closes #1894
Summary by CodeRabbit
New Features
--no-cacheoption to the database download command, allowing users to force a fresh database backup.Documentation
--no-cacheoption.Bug Fixes
Tests