Fixed DB download variables inconsistencies.#2360
Merged
AlexSkrypnyk merged 3 commits intomainfrom Mar 12, 2026
Merged
Conversation
WalkthroughAdds VORTEX_DB_INDEX-based per-database indexing across download-db dispatcher and source scripts, updates CI/docs to set VORTEX_DB_INDEX=2 for DB2, and extends unit tests to exercise indexed-variable resolution and container-registry cache/skip-file behaviors. No public API changes. Changes
Sequence Diagram(s)sequenceDiagram
participant CI as CI Runner
participant Script as download-db.sh
participant Source as source helper (e.g., container_registry)
participant Registry as Container Registry / Docker
CI->>Script: run scripts/vortex/download-db.sh (VORTEX_DB_INDEX=2)
Script->>Script: resolve _db_index from VORTEX_DB_INDEX
Script->>Source: invoke source helper with resolved VORTEX_DOWNLOAD_DB vars
Source->>Registry: docker login (user/password from indexed vars)
Registry-->>Source: auth OK
Source->>Registry: docker pull/pull-by-digest (indexed image)
Registry-->>Source: image layer stream
Source-->>Script: report success (download path, DB dir)
Script-->>CI: exit 0 / success message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
This comment has been minimized.
This comment has been minimized.
Member
Author
|
Member
Author
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2360 +/- ##
==========================================
- Coverage 79.19% 78.92% -0.28%
==========================================
Files 125 118 -7
Lines 6591 6496 -95
Branches 44 0 -44
==========================================
- Hits 5220 5127 -93
+ Misses 1371 1369 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
github-actions
bot
added
the
CONFLICT
AlexSkrypnyk
force-pushed
the
feature/fix-migration-db-download-as-image
branch
from
7073acb to
438885a
Compare
github-actions
bot
removed
the
CONFLICT
This comment has been minimized.
This comment has been minimized.
Member
Author
|
Member
Author
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.ahoy.yml:
- Around line 147-148: The export uses the wrong environment variable name:
change the exported variable VORTEX_DOWNLOAD_DB_FRESH to
VORTEX_DOWNLOAD_DB_FORCE so the case that detects the " --fresh " flag actually
sets the variable the download-db.sh script reads; update the export statement
(the case handling for " --fresh ") to export VORTEX_DOWNLOAD_DB_FORCE to ensure
download-db.sh's check for VORTEX_DOWNLOAD_DB_FORCE will trigger a forced
download.
In @.vortex/tests/bats/unit/download-db.bats:
- Around line 124-185: The tests for indexed variable resolution don't exercise
the path lookup because download-db-url.sh is stubbed and ls is mocked without
checking arguments; update the tests so they either (A) create a real cached
file at the resolved path (use VORTEX_DOWNLOAD_DB2_DIR or VORTEX_DB2_DIR +
VORTEX_DB2_FILE) before running scripts/vortex/download-db.sh and assert the
mock ls/copy was invoked with that path, or (B) allow the real
scripts/vortex/download-db-url.sh to run while mocking external network tools
(curl/unzip) so the helper's file lookup runs; target the test cases that set
VORTEX_DB_INDEX and the variables VORTEX_DOWNLOAD_DB2_DIR, VORTEX_DB2_DIR,
VORTEX_DB2_FILE and update or add assertions against the mocked commands
(mock_command "ls" or mocked curl/unzip) to confirm the resolved file path was
used.
In `@scripts/vortex/download-db.sh`:
- Around line 33-36: The cache check uses VORTEX_DOWNLOAD_DB_FILE and
VORTEX_DOWNLOAD_DB_DIR but those are only resolving generic vars; if a
source-specific helper (scripts/vortex/download-db-url.sh,
download-db-lagoon.sh, download-db-acquia.sh, download-db-ftp.sh) sets its own
indexed DIR/FILE vars the script will miss the real path and re-download. Fix by
resolving the effective directory and filename from the chosen source-specific
indexed variables before the cache gate and final ls: after selecting/including
the helper (or immediately after helper-specific variable assignments), compute
and export VORTEX_DOWNLOAD_DB_DIR and VORTEX_DOWNLOAD_DB_FILE from the
source-specific indexed names the helper uses so the subsequent cache check and
ls operate on the actual dump path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5b825fd9-63a8-463a-81a6-b12598904395
⛔ Files ignored due to path filters (17)
.vortex/installer/tests/Fixtures/handler_process/migration_download_source_acquia/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_download_source_acquia/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_download_source_ftp/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_download_source_ftp/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_download_source_lagoon/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_download_source_lagoon/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_download_source_s3/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_download_source_s3/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_download_source_url/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_download_source_url/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_enabled/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_enabled/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_enabled_circleci/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_enabled_circleci/.circleci/config.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_enabled_lagoon/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_enabled_lagoon/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_enabled_lagoon/.lagoon.ymlis excluded by!.vortex/installer/tests/Fixtures/**
📒 Files selected for processing (16)
.ahoy.yml.circleci/config.yml.circleci/vortex-test-common.yml.github/workflows/build-test-deploy.yml.lagoon.yml.vortex/docs/content/development/variables.mdx.vortex/docs/content/drupal/migrations.mdx.vortex/tests/bats/unit/download-db-container-registry.bats.vortex/tests/bats/unit/download-db.batsscripts/vortex/download-db-acquia.shscripts/vortex/download-db-container-registry.shscripts/vortex/download-db-ftp.shscripts/vortex/download-db-lagoon.shscripts/vortex/download-db-s3.shscripts/vortex/download-db-url.shscripts/vortex/download-db.sh
…container_registry` is chosen.
AlexSkrypnyk
force-pushed
the
feature/fix-migration-db-download-as-image
branch
from
438885a to
1cb2a85
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/vortex/download-db-container-registry.sh (1)
69-69: 🧹 Nitpick | 🔵 TrivialError message could be more helpful when using indexed variables.
When
VORTEX_DB_INDEXis set (e.g.,2), users would setVORTEX_DB2_IMAGE, but the error message only mentionsVORTEX_DB_IMAGE. Consider making the message dynamic to include the indexed variant.💡 Suggested improvement
-[ -z "${VORTEX_DOWNLOAD_DB_CONTAINER_REGISTRY_IMAGE}" ] && fail "Destination image name is not specified. Please provide VORTEX_DOWNLOAD_DB_CONTAINER_REGISTRY_IMAGE or VORTEX_DB_IMAGE in a format <org>/<repository>." && exit 1 +[ -z "${VORTEX_DOWNLOAD_DB_CONTAINER_REGISTRY_IMAGE}" ] && fail "Destination image name is not specified. Please provide VORTEX_DOWNLOAD_DB${_db_index}_CONTAINER_REGISTRY_IMAGE or VORTEX_DB${_db_index}_IMAGE in a format <org>/<repository>." && exit 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/vortex/download-db-container-registry.sh` at line 69, Update the error message so it mentions the indexed environment variable when VORTEX_DB_INDEX is set: check VORTEX_DB_INDEX and construct the variable name VORTEX_DB${VORTEX_DB_INDEX}_IMAGE (falling back to VORTEX_DB_IMAGE) and use that name in the fail call for the VORTEX_DOWNLOAD_DB_CONTAINER_REGISTRY_IMAGE check; change the fail text to include both the dynamic indexed variant and the base VORTEX_DB_IMAGE so users know which env var to set (referencing VORTEX_DOWNLOAD_DB_CONTAINER_REGISTRY_IMAGE, VORTEX_DB_INDEX, and VORTEX_DB{index}_IMAGE)..ahoy.yml (1)
139-139:⚠️ Potential issue | 🔴 CriticalFix environment variable name from
VORTEX_DOWNLOAD_DB_FRESHtoVORTEX_DOWNLOAD_DB_FORCE.The
download-db.shscript checks forVORTEX_DOWNLOAD_DB_FORCE(line 28-29), but this line exportsVORTEX_DOWNLOAD_DB_FRESH. The--freshflag will have no effect for the primary database download. Line 147 correctly usesVORTEX_DOWNLOAD_DB_FORCEfordownload-db2.🐛 Proposed fix
- case " $* " in *" --fresh "*) export VORTEX_DOWNLOAD_DB_FRESH=1;; esac + case " $* " in *" --fresh "*) export VORTEX_DOWNLOAD_DB_FORCE=1;; esac🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.ahoy.yml at line 139, The export uses the wrong env var name (VORTEX_DOWNLOAD_DB_FRESH) so the --fresh flag doesn't affect download-db; update the case branch that matches "--fresh" to export VORTEX_DOWNLOAD_DB_FORCE instead of VORTEX_DOWNLOAD_DB_FRESH (preserving the existing case pattern containing "--fresh") so the download-db script reads the intended VORTEX_DOWNLOAD_DB_FORCE flag consistently with download-db2 and the check in download-db.
♻️ Duplicate comments (1)
.vortex/tests/bats/unit/download-db.bats (1)
124-185: 🧹 Nitpick | 🔵 TrivialTests verify indexed variable selection but not full path resolution.
These tests confirm that
VORTEX_DB_INDEX=2triggers the correct source selection and log messages. They don't fully exercise the path resolution (e.g., cache hit at indexed paths) because the URL script is stubbed. This is acceptable for unit test scope, but consider adding a test that creates a file at the indexed path and verifies the cache hit logic works with indexed variables.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.vortex/tests/bats/unit/download-db.bats around lines 124 - 185, Add a unit test that verifies cache-hit path resolution for indexed variables: set VORTEX_DB_INDEX=2 and export the indexed path vars (either VORTEX_DOWNLOAD_DB2_DIR/VORTEX_DOWNLOAD_DB2_FILE or shorthand VORTEX_DB2_DIR/VORTEX_DB2_FILE), create a real file at that path before running scripts/vortex/download-db.sh, run the script, and assert the script reports the cache-hit (the message your download-db.sh emits when using an existing file) and completes successfully; reference the existing test names and the script symbol scripts/vortex/download-db.sh and the env vars VORTEX_DB_INDEX, VORTEX_DOWNLOAD_DB2_*, and VORTEX_DB2_* to locate where to add this new test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.circleci/config.yml:
- Around line 259-262: The migration dump filename fallback is inconsistent:
download-db.sh when VORTEX_DB_INDEX=2 will resolve VORTEX_DB2_FILE or
VORTEX_DOWNLOAD_DB2_FILE, but the later build/copy step only checks
VORTEX_DOWNLOAD_DB2_FILE; update that later step to use the same resolution
logic as download-db.sh (i.e., check VORTEX_DB{index}_FILE first and fall back
to VORTEX_DOWNLOAD_DB{index}_FILE or explicitly check both VORTEX_DB2_FILE and
VORTEX_DOWNLOAD_DB2_FILE) so shorthand-indexed configs that set VORTEX_DB2_FILE
still result in the migration dump being copied into the CLI container.
In @.github/workflows/build-test-deploy.yml:
- Around line 266-269: The provision step currently copies only the fallback
${VORTEX_DOWNLOAD_DB2_FILE:-db2.sql}, which breaks shorthand-indexed downloads;
update that copy/provision logic to mirror the indexed resolution used by the
download step (honor VORTEX_DB_INDEX and prefer VORTEX_DOWNLOAD_DB2_FILE, then
VORTEX_DB2_FILE, then the default db2.sql) so shorthand/indexed configs succeed
end-to-end.
In @.vortex/docs/content/drupal/migrations.mdx:
- Around line 94-98: Update the sentence describing indexed resolution for the
download-db2 command (referencing VORTEX_DB_INDEX=2, VORTEX_DOWNLOAD_DB2_SOURCE,
VORTEX_DB2_IMAGE and the download-db.sh/download-db2 flow) to include
"container_registry" in the list of supported sources alongside URL, FTP,
Acquia, Lagoon, and S3 so the docs accurately reflect that container_registry is
supported for migration DB downloads.
---
Outside diff comments:
In @.ahoy.yml:
- Line 139: The export uses the wrong env var name (VORTEX_DOWNLOAD_DB_FRESH) so
the --fresh flag doesn't affect download-db; update the case branch that matches
"--fresh" to export VORTEX_DOWNLOAD_DB_FORCE instead of VORTEX_DOWNLOAD_DB_FRESH
(preserving the existing case pattern containing "--fresh") so the download-db
script reads the intended VORTEX_DOWNLOAD_DB_FORCE flag consistently with
download-db2 and the check in download-db.
In `@scripts/vortex/download-db-container-registry.sh`:
- Line 69: Update the error message so it mentions the indexed environment
variable when VORTEX_DB_INDEX is set: check VORTEX_DB_INDEX and construct the
variable name VORTEX_DB${VORTEX_DB_INDEX}_IMAGE (falling back to
VORTEX_DB_IMAGE) and use that name in the fail call for the
VORTEX_DOWNLOAD_DB_CONTAINER_REGISTRY_IMAGE check; change the fail text to
include both the dynamic indexed variant and the base VORTEX_DB_IMAGE so users
know which env var to set (referencing
VORTEX_DOWNLOAD_DB_CONTAINER_REGISTRY_IMAGE, VORTEX_DB_INDEX, and
VORTEX_DB{index}_IMAGE).
---
Duplicate comments:
In @.vortex/tests/bats/unit/download-db.bats:
- Around line 124-185: Add a unit test that verifies cache-hit path resolution
for indexed variables: set VORTEX_DB_INDEX=2 and export the indexed path vars
(either VORTEX_DOWNLOAD_DB2_DIR/VORTEX_DOWNLOAD_DB2_FILE or shorthand
VORTEX_DB2_DIR/VORTEX_DB2_FILE), create a real file at that path before running
scripts/vortex/download-db.sh, run the script, and assert the script reports the
cache-hit (the message your download-db.sh emits when using an existing file)
and completes successfully; reference the existing test names and the script
symbol scripts/vortex/download-db.sh and the env vars VORTEX_DB_INDEX,
VORTEX_DOWNLOAD_DB2_*, and VORTEX_DB2_* to locate where to add this new test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5190f3f7-6817-4525-b5cc-9997030c4aad
⛔ Files ignored due to path filters (18)
.vortex/installer/tests/Fixtures/handler_process/migration_download_source_acquia/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_download_source_acquia/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_download_source_ftp/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_download_source_ftp/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_download_source_lagoon/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_download_source_lagoon/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_download_source_s3/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_download_source_s3/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_download_source_url/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_download_source_url/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_enabled/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_enabled/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_enabled_circleci/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_enabled_circleci/.circleci/config.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_enabled_lagoon/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_enabled_lagoon/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_enabled_lagoon/.lagoon.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/tests/composer.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
.ahoy.yml.circleci/config.yml.circleci/vortex-test-common.yml.github/workflows/build-test-deploy.yml.lagoon.yml.vortex/docs/.utils/update-docs.sh.vortex/docs/content/development/variables.mdx.vortex/docs/content/drupal/migrations.mdx.vortex/tests/bats/unit/download-db-container-registry.bats.vortex/tests/bats/unit/download-db.bats.vortex/tests/composer.jsonscripts/vortex/download-db-acquia.shscripts/vortex/download-db-container-registry.shscripts/vortex/download-db-ftp.shscripts/vortex/download-db-lagoon.shscripts/vortex/download-db-s3.shscripts/vortex/download-db-url.shscripts/vortex/download-db.sh
Comment on lines
259
to
262
| - run: | ||
| name: Download migration DB | ||
| command: VORTEX_VAR_PREFIX=VORTEX_DOWNLOAD_DB2 ./scripts/vortex/download-db.sh | ||
| command: VORTEX_DB_INDEX=2 ./scripts/vortex/download-db.sh | ||
| no_output_timeout: 30m |
There was a problem hiding this comment.
Keep the migration dump filename fallback consistent with indexed resolution.
After this switches to VORTEX_DB_INDEX=2, download-db.sh can resolve the target file from VORTEX_DB2_FILE as well as VORTEX_DOWNLOAD_DB2_FILE. The later build step on Lines 375-377 still checks only VORTEX_DOWNLOAD_DB2_FILE, so shorthand-indexed configs will download successfully here but skip copying the migration dump into the CLI container.
Suggested follow-up in the later build step
- if [ -f ".data/${VORTEX_DOWNLOAD_DB2_FILE:-db2.sql}" ]; then
+ migration_db_file="${VORTEX_DOWNLOAD_DB2_FILE:-${VORTEX_DB2_FILE:-db2.sql}}"
+ if [ -f ".data/${migration_db_file}" ]; then
docker compose exec -T cli mkdir -p .data
- docker compose cp -L ".data/${VORTEX_DOWNLOAD_DB2_FILE:-db2.sql}" cli:"/app/.data/${VORTEX_DOWNLOAD_DB2_FILE:-db2.sql}"
+ docker compose cp -L ".data/${migration_db_file}" cli:"/app/.data/${migration_db_file}"
fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.circleci/config.yml around lines 259 - 262, The migration dump filename
fallback is inconsistent: download-db.sh when VORTEX_DB_INDEX=2 will resolve
VORTEX_DB2_FILE or VORTEX_DOWNLOAD_DB2_FILE, but the later build/copy step only
checks VORTEX_DOWNLOAD_DB2_FILE; update that later step to use the same
resolution logic as download-db.sh (i.e., check VORTEX_DB{index}_FILE first and
fall back to VORTEX_DOWNLOAD_DB{index}_FILE or explicitly check both
VORTEX_DB2_FILE and VORTEX_DOWNLOAD_DB2_FILE) so shorthand-indexed configs that
set VORTEX_DB2_FILE still result in the migration dump being copied into the CLI
container.
Comment on lines
266
to
269
| #;< MIGRATION | ||
| - name: Download migration DB | ||
| run: VORTEX_VAR_PREFIX=VORTEX_DOWNLOAD_DB2 ./scripts/vortex/download-db.sh | ||
| run: VORTEX_DB_INDEX=2 ./scripts/vortex/download-db.sh | ||
| #;> MIGRATION |
There was a problem hiding this comment.
Mirror the new indexed filename fallback in the build step.
This download step now honors indexed resolution, including VORTEX_DB2_FILE, but the later provision step on Lines 402-404 still copies only ${VORTEX_DOWNLOAD_DB2_FILE:-db2.sql}. That makes shorthand-indexed migration configs fail later even though the download itself succeeds.
Suggested follow-up in the later provision step
- if [ -f ".data/${VORTEX_DOWNLOAD_DB2_FILE:-db2.sql}" ]; then
+ migration_db_file="${VORTEX_DOWNLOAD_DB2_FILE:-${VORTEX_DB2_FILE:-db2.sql}}"
+ if [ -f ".data/${migration_db_file}" ]; then
docker compose exec -T cli mkdir -p .data
- docker compose cp -L ".data/${VORTEX_DOWNLOAD_DB2_FILE:-db2.sql}" cli:"/app/.data/${VORTEX_DOWNLOAD_DB2_FILE:-db2.sql}"
+ docker compose cp -L ".data/${migration_db_file}" cli:"/app/.data/${migration_db_file}"
fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/build-test-deploy.yml around lines 266 - 269, The
provision step currently copies only the fallback
${VORTEX_DOWNLOAD_DB2_FILE:-db2.sql}, which breaks shorthand-indexed downloads;
update that copy/provision logic to mirror the indexed resolution used by the
download step (honor VORTEX_DB_INDEX and prefer VORTEX_DOWNLOAD_DB2_FILE, then
VORTEX_DB2_FILE, then the default db2.sql) so shorthand/indexed configs succeed
end-to-end.
Comment on lines
94
to
+98
| The `download-db2` command reuses the existing `download-db.sh` script with | ||
| `VORTEX_VAR_PREFIX=VORTEX_DOWNLOAD_DB2`, which remaps all `VORTEX_DOWNLOAD_DB2_*` | ||
| variables to `VORTEX_DOWNLOAD_DB_*` so all existing download sources (URL, FTP, | ||
| Acquia, Lagoon, S3) work for the migration database as well. | ||
| `VORTEX_DB_INDEX=2`, which makes all scripts resolve indexed variable names | ||
| (e.g., `VORTEX_DOWNLOAD_DB2_SOURCE` instead of `VORTEX_DOWNLOAD_DB_SOURCE`, | ||
| `VORTEX_DB2_IMAGE` instead of `VORTEX_DB_IMAGE`) so all existing download | ||
| sources (URL, FTP, Acquia, Lagoon, S3) work for the migration database as well. |
There was a problem hiding this comment.
Document the container_registry source here too.
This sentence explains the new indexed resolution, but it still lists only URL/FTP/Acquia/Lagoon/S3. The PR is also fixing migration DB downloads for container_registry, so the source list is left incomplete.
Suggested doc tweak
- sources (URL, FTP, Acquia, Lagoon, S3) work for the migration database as well.
+ sources (URL, FTP, Acquia, Lagoon, S3, container_registry) work for the
+ migration database as well.📝 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.
Suggested change
| The `download-db2` command reuses the existing `download-db.sh` script with | |
| `VORTEX_VAR_PREFIX=VORTEX_DOWNLOAD_DB2`, which remaps all `VORTEX_DOWNLOAD_DB2_*` | |
| variables to `VORTEX_DOWNLOAD_DB_*` so all existing download sources (URL, FTP, | |
| Acquia, Lagoon, S3) work for the migration database as well. | |
| `VORTEX_DB_INDEX=2`, which makes all scripts resolve indexed variable names | |
| (e.g., `VORTEX_DOWNLOAD_DB2_SOURCE` instead of `VORTEX_DOWNLOAD_DB_SOURCE`, | |
| `VORTEX_DB2_IMAGE` instead of `VORTEX_DB_IMAGE`) so all existing download | |
| sources (URL, FTP, Acquia, Lagoon, S3) work for the migration database as well. | |
| The `download-db2` command reuses the existing `download-db.sh` script with | |
| `VORTEX_DB_INDEX=2`, which makes all scripts resolve indexed variable names | |
| (e.g., `VORTEX_DOWNLOAD_DB2_SOURCE` instead of `VORTEX_DOWNLOAD_DB_SOURCE`, | |
| `VORTEX_DB2_IMAGE` instead of `VORTEX_DB_IMAGE`) so all existing download | |
| sources (URL, FTP, Acquia, Lagoon, S3, container_registry) work for the | |
| migration database as well. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.vortex/docs/content/drupal/migrations.mdx around lines 94 - 98, Update the
sentence describing indexed resolution for the download-db2 command (referencing
VORTEX_DB_INDEX=2, VORTEX_DOWNLOAD_DB2_SOURCE, VORTEX_DB2_IMAGE and the
download-db.sh/download-db2 flow) to include "container_registry" in the list of
supported sources alongside URL, FTP, Acquia, Lagoon, and S3 so the docs
accurately reflect that container_registry is supported for migration DB
downloads.
Member
Author
|
Member
Author
|
Member
Author
|
AlexSkrypnyk
force-pushed
the
feature/fix-migration-db-download-as-image
branch
from
1cb2a85 to
17f0f02
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.ahoy.yml (1)
139-140:⚠️ Potential issue | 🔴 CriticalFix environment variable:
VORTEX_DOWNLOAD_DB_FRESHshould beVORTEX_DOWNLOAD_DB_FORCE.The
download-db.shscript checks forVORTEX_DOWNLOAD_DB_FORCE(notFRESH) to force a fresh download. Line 139 exportsVORTEX_DOWNLOAD_DB_FRESH=1, which has no effect. Note that line 147 correctly usesVORTEX_DOWNLOAD_DB_FORCE=1fordownload-db2.🐛 Proposed fix
download-db: usage: Download database. Run with "--fresh" option to force fresh database backup. aliases: [fetch-db] cmd: | - case " $* " in *" --fresh "*) export VORTEX_DOWNLOAD_DB_FRESH=1;; esac + case " $* " in *" --fresh "*) export VORTEX_DOWNLOAD_DB_FORCE=1;; esac ./scripts/vortex/download-db.sh🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.ahoy.yml around lines 139 - 140, The export in the case handling for "--fresh" is using the wrong environment variable name; change the export from VORTEX_DOWNLOAD_DB_FRESH=1 to VORTEX_DOWNLOAD_DB_FORCE=1 so the ./scripts/vortex/download-db.sh script (which checks VORTEX_DOWNLOAD_DB_FORCE) actually receives the flag; ensure this matches the existing VORTEX_DOWNLOAD_DB_FORCE usage elsewhere (e.g., the download-db2 branch).
♻️ Duplicate comments (4)
.circleci/config.yml (1)
375-378:⚠️ Potential issue | 🟠 MajorMigration dump filename fallback inconsistent with download script.
Same issue as in the GitHub workflow: the provision step only checks
${VORTEX_DOWNLOAD_DB2_FILE:-db2.sql}but the download script also honorsVORTEX_DB2_FILE. Apply the same fix pattern.🔧 Suggested fix
#;< MIGRATION - if [ -f ".data/${VORTEX_DOWNLOAD_DB2_FILE:-db2.sql}" ]; then + migration_db_file="${VORTEX_DOWNLOAD_DB2_FILE:-${VORTEX_DB2_FILE:-db2.sql}}" + if [ -f ".data/${migration_db_file}" ]; then docker compose exec -T cli mkdir -p .data - docker compose cp -L ".data/${VORTEX_DOWNLOAD_DB2_FILE:-db2.sql}" cli:"/app/.data/${VORTEX_DOWNLOAD_DB2_FILE:-db2.sql}" + docker compose cp -L ".data/${migration_db_file}" cli:"/app/.data/${migration_db_file}" fi #;> MIGRATION🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.circleci/config.yml around lines 375 - 378, The conditional and cp invocation only use ${VORTEX_DOWNLOAD_DB2_FILE:-db2.sql} but should mirror the download script by also honoring VORTEX_DB2_FILE; update the if test and the docker compose cp source and target expansions to use a nested fallback like ${VORTEX_DOWNLOAD_DB2_FILE:-${VORTEX_DB2_FILE:-db2.sql}} so the mkdir (docker compose exec -T cli mkdir -p .data) and copy (docker compose cp -L "...") operate on the same resolved filename..github/workflows/build-test-deploy.yml (1)
266-269:⚠️ Potential issue | 🟠 MajorMigration dump filename fallback remains inconsistent with download script.
The download step (line 268) now uses
VORTEX_DB_INDEX=2, which resolves the file from eitherVORTEX_DOWNLOAD_DB2_FILEorVORTEX_DB2_FILE. However, the provision step (lines 402-405) still only checks${VORTEX_DOWNLOAD_DB2_FILE:-db2.sql}, missing theVORTEX_DB2_FILEfallback. If a user sets onlyVORTEX_DB2_FILE=custom.sql, the download succeeds but the provision step looks for the wrong file.🔧 Suggested fix to align provision with download resolution
#;< MIGRATION - if [ -f ".data/${VORTEX_DOWNLOAD_DB2_FILE:-db2.sql}" ]; then + migration_db_file="${VORTEX_DOWNLOAD_DB2_FILE:-${VORTEX_DB2_FILE:-db2.sql}}" + if [ -f ".data/${migration_db_file}" ]; then docker compose exec -T cli mkdir -p .data - docker compose cp -L ".data/${VORTEX_DOWNLOAD_DB2_FILE:-db2.sql}" cli:"/app/.data/${VORTEX_DOWNLOAD_DB2_FILE:-db2.sql}" + docker compose cp -L ".data/${migration_db_file}" cli:"/app/.data/${migration_db_file}" fi #;> MIGRATIONAlso applies to: 402-405
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-test-deploy.yml around lines 266 - 269, The provision step's filename fallback must match the download resolution: update the provision logic that currently uses ${VORTEX_DOWNLOAD_DB2_FILE:-db2.sql} to resolve using VORTEX_DB_INDEX=2 semantics (i.e., prefer VORTEX_DOWNLOAD_DB2_FILE, then VORTEX_DB2_FILE, then db2.sql). Locate the provision block that references VORTEX_DOWNLOAD_DB2_FILE for DB2 and change its fallback chain to include VORTEX_DB2_FILE as the second option so both download (VORTEX_DB_INDEX=2) and provision use the same filename resolution.scripts/vortex/download-db.sh (1)
36-43:⚠️ Potential issue | 🟠 MajorSource-specific scripts use separate indexed variables that can diverge from the main script's resolved directory.
The main script resolves
VORTEX_DOWNLOAD_DB_DIR(lines 36–43) usingVORTEX_DOWNLOAD_DB${_db_index}_DIRas the primary lookup. However, source-specific scripts (lagoon, acquia, ftp, url) each define their own source-specific indexed variable (e.g.,VORTEX_DOWNLOAD_DB${_db_index}_LAGOON_DB_DIR) which also falls back toVORTEX_DOWNLOAD_DB${_db_index}_DIR.If a caller sets only the source-specific indexed variable (e.g.,
VORTEX_DOWNLOAD_DB2_LAGOON_DB_DIR=/custom/path) but not the generic one (VORTEX_DOWNLOAD_DB2_DIR), the source-specific script will use the custom path while the main script'sVORTEX_DOWNLOAD_DB_DIRdefaults to./.data. The finallsoutput on line 104 will then display an incorrect directory.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/vortex/download-db.sh` around lines 36 - 43, The VORTEX_DOWNLOAD_DB_DIR assignment only checks VORTEX_DOWNLOAD_DB${_db_index}_DIR and misses source-specific indexed fallbacks (e.g. VORTEX_DOWNLOAD_DB${_db_index}_LAGOON_DB_DIR, VORTEX_DOWNLOAD_DB${_db_index}_ACQUIA_DB_DIR, VORTEX_DOWNLOAD_DB${_db_index}_FTP_DB_DIR, VORTEX_DOWNLOAD_DB${_db_index}_URL_DB_DIR), causing mismatched dirs between source scripts and the main script; update the VORTEX_DOWNLOAD_DB_DIR resolution to prefer any of those source-specific indexed env vars (use indirect expansion like "${!varname}") before falling back to VORTEX_DOWNLOAD_DB${_db_index}_DIR and then ./ .data so the main script and source-specific scripts agree (modify the VORTEX_DOWNLOAD_DB_DIR assignment that currently uses _v/_vs and VORTEX_DOWNLOAD_DB_DIR)..vortex/tests/bats/unit/download-db.bats (1)
124-153:⚠️ Potential issue | 🟡 MinorThese indexed tests still don't exercise the resolved
DIR/FILElookup.Because
download-db-url.shis stubbed and thelsmock is never checked against the resolveddb2.sqlpath, these cases only prove indexed source selection and log text. A regression inVORTEX_DOWNLOAD_DB2_DIR/VORTEX_DB2_DIR/...FILEresolution would still pass here. Please either let the real URL helper run with external tools mocked, or assert that the cached-file lookup uses the indexed path.Also applies to: 155-185
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.vortex/tests/bats/unit/download-db.bats around lines 124 - 153, The test only verifies source selection and messages because scripts/vortex/download-db-url.sh is stubbed and the mock for ls (mock_command "ls") never asserts it was called with the resolved indexed file path; update the test so it either (A) lets the real scripts/vortex/download-db-url.sh run while mocking external tools (e.g., curl/wget) or (B) explicitly asserts the cached-file lookup uses the resolved indexed path by checking that the ls mock was invoked with "${VORTEX_DOWNLOAD_DB2_DIR}/${VORTEX_DOWNLOAD_DB2_FILE}" (i.e., ".data/db2.sql") after exporting VORTEX_DB_INDEX and VORTEX_DOWNLOAD_DB2_DIR/FILE, ensuring scripts/vortex/download-db.sh actually resolves the indexed DIR/FILE variables.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.lagoon.yml:
- Around line 68-71: Change the environment variable from VORTEX_DB_DIR to the
indexed name VORTEX_DB2_DIR so the download script's indirect expansion picks it
up; specifically, replace the export VORTEX_DB_DIR=/tmp/data with export
VORTEX_DB2_DIR=/tmp/data so that when VORTEX_DB_INDEX=2 is set (and the script
./scripts/vortex/download-db.sh looks for VORTEX_DOWNLOAD_DB2_DIR or
VORTEX_DB2_DIR) the override is honored.
In `@scripts/vortex/download-db-acquia.sh`:
- Around line 28-74: The repeated lookup ladder using _v / _vs / _vss to resolve
indexed environment variables should be extracted to a shared helper to avoid
duplication; implement a small function (e.g., get_indexed_env_var or
resolve_indexed_var) that accepts the base name and _db_index and returns the
resolved value (using the same precedence ${!_v:-${!_vs:-${!_vss:-default}}})
and replace the repeated blocks that set VORTEX_DOWNLOAD_DB_ACQUIA_KEY,
VORTEX_DOWNLOAD_DB_ACQUIA_SECRET, VORTEX_DOWNLOAD_DB_ACQUIA_APP_NAME,
VORTEX_DOWNLOAD_DB_ENVIRONMENT, VORTEX_DOWNLOAD_DB_ACQUIA_DB_NAME,
VORTEX_DOWNLOAD_DB_ACQUIA_DB_DIR, VORTEX_DOWNLOAD_DB_ACQUIA_DB_FILE,
VORTEX_DOWNLOAD_DB_FRESH, VORTEX_DOWNLOAD_DB_ACQUIA_BACKUP_WAIT_INTERVAL and
VORTEX_DOWNLOAD_DB_ACQUIA_BACKUP_MAX_WAIT to call that helper; place the helper
in a common sourced script (or top of this script) and ensure it preserves
existing fallback semantics and defaults.
---
Outside diff comments:
In @.ahoy.yml:
- Around line 139-140: The export in the case handling for "--fresh" is using
the wrong environment variable name; change the export from
VORTEX_DOWNLOAD_DB_FRESH=1 to VORTEX_DOWNLOAD_DB_FORCE=1 so the
./scripts/vortex/download-db.sh script (which checks VORTEX_DOWNLOAD_DB_FORCE)
actually receives the flag; ensure this matches the existing
VORTEX_DOWNLOAD_DB_FORCE usage elsewhere (e.g., the download-db2 branch).
---
Duplicate comments:
In @.circleci/config.yml:
- Around line 375-378: The conditional and cp invocation only use
${VORTEX_DOWNLOAD_DB2_FILE:-db2.sql} but should mirror the download script by
also honoring VORTEX_DB2_FILE; update the if test and the docker compose cp
source and target expansions to use a nested fallback like
${VORTEX_DOWNLOAD_DB2_FILE:-${VORTEX_DB2_FILE:-db2.sql}} so the mkdir (docker
compose exec -T cli mkdir -p .data) and copy (docker compose cp -L "...")
operate on the same resolved filename.
In @.github/workflows/build-test-deploy.yml:
- Around line 266-269: The provision step's filename fallback must match the
download resolution: update the provision logic that currently uses
${VORTEX_DOWNLOAD_DB2_FILE:-db2.sql} to resolve using VORTEX_DB_INDEX=2
semantics (i.e., prefer VORTEX_DOWNLOAD_DB2_FILE, then VORTEX_DB2_FILE, then
db2.sql). Locate the provision block that references VORTEX_DOWNLOAD_DB2_FILE
for DB2 and change its fallback chain to include VORTEX_DB2_FILE as the second
option so both download (VORTEX_DB_INDEX=2) and provision use the same filename
resolution.
In @.vortex/tests/bats/unit/download-db.bats:
- Around line 124-153: The test only verifies source selection and messages
because scripts/vortex/download-db-url.sh is stubbed and the mock for ls
(mock_command "ls") never asserts it was called with the resolved indexed file
path; update the test so it either (A) lets the real
scripts/vortex/download-db-url.sh run while mocking external tools (e.g.,
curl/wget) or (B) explicitly asserts the cached-file lookup uses the resolved
indexed path by checking that the ls mock was invoked with
"${VORTEX_DOWNLOAD_DB2_DIR}/${VORTEX_DOWNLOAD_DB2_FILE}" (i.e., ".data/db2.sql")
after exporting VORTEX_DB_INDEX and VORTEX_DOWNLOAD_DB2_DIR/FILE, ensuring
scripts/vortex/download-db.sh actually resolves the indexed DIR/FILE variables.
In `@scripts/vortex/download-db.sh`:
- Around line 36-43: The VORTEX_DOWNLOAD_DB_DIR assignment only checks
VORTEX_DOWNLOAD_DB${_db_index}_DIR and misses source-specific indexed fallbacks
(e.g. VORTEX_DOWNLOAD_DB${_db_index}_LAGOON_DB_DIR,
VORTEX_DOWNLOAD_DB${_db_index}_ACQUIA_DB_DIR,
VORTEX_DOWNLOAD_DB${_db_index}_FTP_DB_DIR,
VORTEX_DOWNLOAD_DB${_db_index}_URL_DB_DIR), causing mismatched dirs between
source scripts and the main script; update the VORTEX_DOWNLOAD_DB_DIR resolution
to prefer any of those source-specific indexed env vars (use indirect expansion
like "${!varname}") before falling back to VORTEX_DOWNLOAD_DB${_db_index}_DIR
and then ./ .data so the main script and source-specific scripts agree (modify
the VORTEX_DOWNLOAD_DB_DIR assignment that currently uses _v/_vs and
VORTEX_DOWNLOAD_DB_DIR).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 355e75af-49d6-4267-9c24-08e60cce9ba4
⛔ Files ignored due to path filters (18)
.vortex/installer/tests/Fixtures/handler_process/migration_download_source_acquia/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_download_source_acquia/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_download_source_ftp/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_download_source_ftp/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_download_source_lagoon/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_download_source_lagoon/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_download_source_s3/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_download_source_s3/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_download_source_url/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_download_source_url/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_enabled/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_enabled/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_enabled_circleci/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_enabled_circleci/.circleci/config.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_enabled_lagoon/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_enabled_lagoon/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/migration_enabled_lagoon/.lagoon.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/tests/composer.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
.ahoy.yml.circleci/config.yml.circleci/vortex-test-common.yml.github/workflows/build-test-deploy.yml.lagoon.yml.vortex/docs/.utils/update-docs.sh.vortex/docs/content/development/variables.mdx.vortex/docs/content/drupal/migrations.mdx.vortex/tests/bats/unit/download-db-container-registry.bats.vortex/tests/bats/unit/download-db.bats.vortex/tests/composer.jsonscripts/vortex/download-db-acquia.shscripts/vortex/download-db-container-registry.shscripts/vortex/download-db-ftp.shscripts/vortex/download-db-lagoon.shscripts/vortex/download-db-s3.shscripts/vortex/download-db-url.shscripts/vortex/download-db.sh
Comment on lines
68
to
71
| export VORTEX_DOWNLOAD_DB_SSH_FILE=false | ||
| export VORTEX_DB_DIR=/tmp/data | ||
| VORTEX_VAR_PREFIX=VORTEX_DOWNLOAD_DB2 ./scripts/vortex/download-db.sh | ||
| VORTEX_DB_INDEX=2 ./scripts/vortex/download-db.sh | ||
| service: cli |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how download-db.sh resolves DIR with index
echo "=== DIR resolution in download-db.sh ==="
sed -n '40,44p' scripts/vortex/download-db.sh
echo ""
echo "=== Check if non-indexed fallback exists ==="
rg -n "VORTEX_DB_DIR" scripts/vortex/download-db.sh | head -10Repository: drevops/vortex
Length of output: 304
🏁 Script executed:
#!/bin/bash
# Check how _db_index is derived from VORTEX_DB_INDEX
echo "=== _db_index assignment in download-db.sh ==="
rg -n "_db_index" scripts/vortex/download-db.sh | head -20Repository: drevops/vortex
Length of output: 566
Change VORTEX_DB_DIR to VORTEX_DB2_DIR on line 69.
Line 69 sets VORTEX_DB_DIR=/tmp/data, but the download script uses bash indirect variable expansion to construct indexed variable names. When VORTEX_DB_INDEX=2 on line 70, the script resolves the directory from VORTEX_DOWNLOAD_DB2_DIR or VORTEX_DB2_DIR, not the non-indexed VORTEX_DB_DIR. The /tmp/data override will be ignored, causing the migration database to download to an unintended location. Use VORTEX_DB2_DIR=/tmp/data instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.lagoon.yml around lines 68 - 71, Change the environment variable from
VORTEX_DB_DIR to the indexed name VORTEX_DB2_DIR so the download script's
indirect expansion picks it up; specifically, replace the export
VORTEX_DB_DIR=/tmp/data with export VORTEX_DB2_DIR=/tmp/data so that when
VORTEX_DB_INDEX=2 is set (and the script ./scripts/vortex/download-db.sh looks
for VORTEX_DOWNLOAD_DB2_DIR or VORTEX_DB2_DIR) the override is honored.
Comment on lines
+28
to
+74
| # Database index suffix. When set (e.g., "2"), all DB-related variable lookups | ||
| # use the indexed variant. | ||
| _db_index="${VORTEX_DB_INDEX:-}" | ||
|
|
||
| # Acquia Cloud API key. | ||
| VORTEX_DOWNLOAD_DB_ACQUIA_KEY="${VORTEX_DOWNLOAD_DB_ACQUIA_KEY:-${VORTEX_ACQUIA_KEY:-}}" | ||
| _v="VORTEX_DOWNLOAD_DB${_db_index}_ACQUIA_KEY" | ||
| VORTEX_DOWNLOAD_DB_ACQUIA_KEY="${!_v:-${VORTEX_ACQUIA_KEY:-}}" | ||
|
|
||
| # Acquia Cloud API secret. | ||
| VORTEX_DOWNLOAD_DB_ACQUIA_SECRET="${VORTEX_DOWNLOAD_DB_ACQUIA_SECRET:-${VORTEX_ACQUIA_SECRET:-}}" | ||
| _v="VORTEX_DOWNLOAD_DB${_db_index}_ACQUIA_SECRET" | ||
| VORTEX_DOWNLOAD_DB_ACQUIA_SECRET="${!_v:-${VORTEX_ACQUIA_SECRET:-}}" | ||
|
|
||
| # Application name. Used to discover UUID. | ||
| VORTEX_DOWNLOAD_DB_ACQUIA_APP_NAME="${VORTEX_DOWNLOAD_DB_ACQUIA_APP_NAME:-${VORTEX_ACQUIA_APP_NAME:-}}" | ||
| _v="VORTEX_DOWNLOAD_DB${_db_index}_ACQUIA_APP_NAME" | ||
| VORTEX_DOWNLOAD_DB_ACQUIA_APP_NAME="${!_v:-${VORTEX_ACQUIA_APP_NAME:-}}" | ||
|
|
||
| # Source environment name used to download the database dump from. | ||
| VORTEX_DOWNLOAD_DB_ENVIRONMENT="${VORTEX_DOWNLOAD_DB_ENVIRONMENT:-}" | ||
| _v="VORTEX_DOWNLOAD_DB${_db_index}_ENVIRONMENT" | ||
| VORTEX_DOWNLOAD_DB_ENVIRONMENT="${!_v:-}" | ||
|
|
||
| # Database name within source environment used to download the database dump. | ||
| VORTEX_DOWNLOAD_DB_ACQUIA_DB_NAME="${VORTEX_DOWNLOAD_DB_ACQUIA_DB_NAME:-}" | ||
| _v="VORTEX_DOWNLOAD_DB${_db_index}_ACQUIA_DB_NAME" | ||
| VORTEX_DOWNLOAD_DB_ACQUIA_DB_NAME="${!_v:-}" | ||
|
|
||
| # Directory where DB dumps are stored. | ||
| VORTEX_DOWNLOAD_DB_ACQUIA_DB_DIR="${VORTEX_DOWNLOAD_DB_ACQUIA_DB_DIR:-${VORTEX_DOWNLOAD_DB_DIR:-${VORTEX_DB_DIR:-./.data}}}" | ||
| _v="VORTEX_DOWNLOAD_DB${_db_index}_ACQUIA_DB_DIR" | ||
| _vs="VORTEX_DOWNLOAD_DB${_db_index}_DIR" | ||
| _vss="VORTEX_DB${_db_index}_DIR" | ||
| VORTEX_DOWNLOAD_DB_ACQUIA_DB_DIR="${!_v:-${!_vs:-${!_vss:-./.data}}}" | ||
|
|
||
| # Database dump file name. | ||
| VORTEX_DOWNLOAD_DB_ACQUIA_DB_FILE="${VORTEX_DOWNLOAD_DB_ACQUIA_DB_FILE:-${VORTEX_DOWNLOAD_DB_FILE:-${VORTEX_DB_FILE:-db.sql}}}" | ||
| _v="VORTEX_DOWNLOAD_DB${_db_index}_ACQUIA_DB_FILE" | ||
| _vs="VORTEX_DOWNLOAD_DB${_db_index}_FILE" | ||
| _vss="VORTEX_DB${_db_index}_FILE" | ||
| VORTEX_DOWNLOAD_DB_ACQUIA_DB_FILE="${!_v:-${!_vs:-${!_vss:-db.sql}}}" | ||
|
|
||
| # Flag to download a fresh copy of the database by triggering a new backup. | ||
| VORTEX_DOWNLOAD_DB_FRESH="${VORTEX_DOWNLOAD_DB_FRESH:-}" | ||
| _v="VORTEX_DOWNLOAD_DB${_db_index}_FRESH" | ||
| VORTEX_DOWNLOAD_DB_FRESH="${!_v:-}" | ||
|
|
||
| # Interval in seconds to wait between backup status checks. | ||
| VORTEX_DOWNLOAD_DB_ACQUIA_BACKUP_WAIT_INTERVAL="${VORTEX_DOWNLOAD_DB_ACQUIA_BACKUP_WAIT_INTERVAL:-10}" | ||
| _v="VORTEX_DOWNLOAD_DB${_db_index}_ACQUIA_BACKUP_WAIT_INTERVAL" | ||
| VORTEX_DOWNLOAD_DB_ACQUIA_BACKUP_WAIT_INTERVAL="${!_v:-10}" | ||
|
|
||
| # Maximum time in seconds to wait for backup completion. | ||
| VORTEX_DOWNLOAD_DB_ACQUIA_BACKUP_MAX_WAIT="${VORTEX_DOWNLOAD_DB_ACQUIA_BACKUP_MAX_WAIT:-600}" | ||
| _v="VORTEX_DOWNLOAD_DB${_db_index}_ACQUIA_BACKUP_MAX_WAIT" | ||
| VORTEX_DOWNLOAD_DB_ACQUIA_BACKUP_MAX_WAIT="${!_v:-600}" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract indexed env resolution into a shared helper.
The _v / _vs / _vss lookup ladder is now copied across every download-db-*.sh variant in this PR. That makes the next variable rename or fallback tweak easy to fix in one script and miss in another.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/vortex/download-db-acquia.sh` around lines 28 - 74, The repeated
lookup ladder using _v / _vs / _vss to resolve indexed environment variables
should be extracted to a shared helper to avoid duplication; implement a small
function (e.g., get_indexed_env_var or resolve_indexed_var) that accepts the
base name and _db_index and returns the resolved value (using the same
precedence ${!_v:-${!_vs:-${!_vss:-default}}}) and replace the repeated blocks
that set VORTEX_DOWNLOAD_DB_ACQUIA_KEY, VORTEX_DOWNLOAD_DB_ACQUIA_SECRET,
VORTEX_DOWNLOAD_DB_ACQUIA_APP_NAME, VORTEX_DOWNLOAD_DB_ENVIRONMENT,
VORTEX_DOWNLOAD_DB_ACQUIA_DB_NAME, VORTEX_DOWNLOAD_DB_ACQUIA_DB_DIR,
VORTEX_DOWNLOAD_DB_ACQUIA_DB_FILE, VORTEX_DOWNLOAD_DB_FRESH,
VORTEX_DOWNLOAD_DB_ACQUIA_BACKUP_WAIT_INTERVAL and
VORTEX_DOWNLOAD_DB_ACQUIA_BACKUP_MAX_WAIT to call that helper; place the helper
in a common sourced script (or top of this script) and ensure it preserves
existing fallback semantics and defaults.
|
Member
Author
|
Member
Author
|
Member
Author
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Tests
New Features
Chores