Ported recent shell script changes to PHP tooling package.#2415
Merged
AlexSkrypnyk merged 2 commits intoproject/2.xfrom Mar 24, 2026
Merged
Ported recent shell script changes to PHP tooling package.#2415AlexSkrypnyk merged 2 commits intoproject/2.xfrom
AlexSkrypnyk merged 2 commits intoproject/2.xfrom
Conversation
WalkthroughAdds index-aware database configuration support across download-db scripts, replaces Composer-based git-artifact install with a direct GitHub release binary download plus SHA256 verification, and introduces fallback-specific behavior in provisioning with related test updates. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## project/2.x #2415 +/- ##
============================================
Coverage 78.93% 78.93%
============================================
Files 119 119
Lines 6565 6565
============================================
Hits 5182 5182
Misses 1383 1383 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.vortex/tooling/src/download-db-container-registry:
- Around line 20-41: The fallback ordering for image selection in the
getenv_required call for $image (function getenv_required with the combined keys
for VORTEX_DOWNLOAD_DB..._CONTAINER_REGISTRY_IMAGE,
VORTEX_DOWNLOAD_DB_CONTAINER_REGISTRY_IMAGE, VORTEX_DB{$db_index}_IMAGE,
VORTEX_DB_IMAGE) and for $image_base (getenv_default with the combined keys for
VORTEX_DOWNLOAD_DB..._CONTAINER_REGISTRY_IMAGE_BASE,
VORTEX_DOWNLOAD_DB_CONTAINER_REGISTRY_IMAGE_BASE,
VORTEX_DB{$db_index}_IMAGE_BASE, VORTEX_DB_IMAGE_BASE) is wrong: move the
indexed keys 'VORTEX_DB' . $db_index . '_IMAGE' and 'VORTEX_DB' . $db_index .
'_IMAGE_BASE' earlier in each array so they are checked before the legacy
VORTEX_DOWNLOAD_DB_CONTAINER_REGISTRY_* vars, ensuring per-index overrides win;
update the array_values(array_unique([...])) arguments for $image and
$image_base accordingly.
In @.vortex/tooling/src/download-db-lagoon:
- Around line 50-51: The code reads $ssh_fingerprint via getenv_default but
never uses it, while the SSH invocation still forces
UserKnownHostsFile=/dev/null and StrictHostKeyChecking=no, so indexed
fingerprints are a no-op; fix by honoring $ssh_fingerprint in the SSH flow: if
$ssh_fingerprint (from getenv_default) is set, create a temporary known_hosts
file containing that fingerprint and update the SSH/scp command options to use
-o UserKnownHostsFile=/path/to/tmp_known_hosts and -o StrictHostKeyChecking=yes
(otherwise keep the current /dev/null + no checking behavior), and ensure the
temp file is cleaned up after use so the indexed
VORTEX_DOWNLOAD_DB{index}_SSH_FINGERPRINT actually affects host verification.
In @.vortex/tooling/src/provision:
- Around line 465-467: The current use of the Elvis operator with
getenv('VORTEX_PROVISION_POST_OPERATIONS_SKIP') incorrectly treats the string
'0' as falsy and falls back to $provision_post_operations_skip; change the
assignment to explicitly detect getenv() returning false and use the environment
value when it is any string (including '0') — e.g. fetch
getenv('VORTEX_PROVISION_POST_OPERATIONS_SKIP') into a temporary, check !==
false, and assign that value to $provision_post_operations_skip if present; this
touches the getenv call and the $provision_post_operations_skip assignment and
ensures provision_from_profile() semantics are preserved.
In @.vortex/tooling/tests/Unit/DeployArtifactTest.php:
- Around line 395-410: The mock in mockGitArtifactDownload() hardcodes the
git-artifact version "1.4.0" which can drift from the real default; extract that
literal into a shared class constant (e.g., DEFAULT_GIT_ARTIFACT_VERSION) and
use that constant in mockGitArtifactDownload() when building the download URL
and anywhere else (createDummyGitArtifactBinary or related tests) that needs the
version so the test remains in sync with the production default; alternatively
read the default from the same production constant/source and reference it in
mockGitArtifactDownload() and createDummyGitArtifactBinary().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2aac9fed-f341-475b-b556-b8847db7e3bf
📒 Files selected for processing (12)
.vortex/tooling/src/deploy-artifact.vortex/tooling/src/download-db.vortex/tooling/src/download-db-acquia.vortex/tooling/src/download-db-container-registry.vortex/tooling/src/download-db-ftp.vortex/tooling/src/download-db-lagoon.vortex/tooling/src/download-db-s3.vortex/tooling/src/download-db-url.vortex/tooling/src/notify.vortex/tooling/src/provision.vortex/tooling/tests/Unit/DeployArtifactTest.php.vortex/tooling/tests/Unit/ProvisionTest.php
Comment on lines
+50
to
+51
| // SSH key fingerprint used to connect to a remote. | ||
| $ssh_fingerprint = getenv_default(...array_values(array_unique(['VORTEX_DOWNLOAD_DB' . $db_index . '_SSH_FINGERPRINT', 'VORTEX_DOWNLOAD_DB_SSH_FINGERPRINT', '']))); |
There was a problem hiding this comment.
VORTEX_DOWNLOAD_DB{index}_SSH_FINGERPRINT is a no-op here.
$ssh_fingerprint is never consumed after Line 51, and the later SSH call still uses UserKnownHostsFile=/dev/null plus StrictHostKeyChecking=no. As written, an indexed fingerprint cannot affect host verification, so this new config path does not work.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.vortex/tooling/src/download-db-lagoon around lines 50 - 51, The code reads
$ssh_fingerprint via getenv_default but never uses it, while the SSH invocation
still forces UserKnownHostsFile=/dev/null and StrictHostKeyChecking=no, so
indexed fingerprints are a no-op; fix by honoring $ssh_fingerprint in the SSH
flow: if $ssh_fingerprint (from getenv_default) is set, create a temporary
known_hosts file containing that fingerprint and update the SSH/scp command
options to use -o UserKnownHostsFile=/path/to/tmp_known_hosts and -o
StrictHostKeyChecking=yes (otherwise keep the current /dev/null + no checking
behavior), and ensure the temp file is cleaned up after use so the indexed
VORTEX_DOWNLOAD_DB{index}_SSH_FINGERPRINT actually affects host verification.
Comment on lines
+395
to
+410
| protected function createDummyGitArtifactBinary(): void { | ||
| $bin_path = $this->getGitArtifactBinPath(); | ||
| file_put_contents($bin_path, ''); | ||
| $this->envSet('VORTEX_DEPLOY_ARTIFACT_GIT_ARTIFACT_SHA256', 'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855'); | ||
| } | ||
|
|
||
| /** | ||
| * Mock the git-artifact download request. | ||
| */ | ||
| protected function mockGitArtifactDownload(): void { | ||
| $this->mockRequest( | ||
| 'https://github.com/drevops/git-artifact/releases/download/1.4.0/git-artifact', | ||
| ['method' => 'GET'], | ||
| ['ok' => TRUE, 'status' => 200, 'body' => ''], | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting hardcoded version to a class constant.
The version 1.4.0 is hardcoded in mockGitArtifactDownload() (line 406) and matches the script's default. If the default version changes, this test mock will silently fail to match. Consider extracting to a shared constant or reading from the same default.
♻️ Suggested improvement
+ protected const GIT_ARTIFACT_VERSION = '1.4.0';
+
protected function createDummyGitArtifactBinary(): void {
$bin_path = $this->getGitArtifactBinPath();
file_put_contents($bin_path, '');
$this->envSet('VORTEX_DEPLOY_ARTIFACT_GIT_ARTIFACT_SHA256', 'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855');
}
protected function mockGitArtifactDownload(): void {
$this->mockRequest(
- 'https://github.com/drevops/git-artifact/releases/download/1.4.0/git-artifact',
+ sprintf('https://github.com/drevops/git-artifact/releases/download/%s/git-artifact', self::GIT_ARTIFACT_VERSION),
['method' => 'GET'],
['ok' => TRUE, 'status' => 200, 'body' => ''],
);
}📝 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
| protected function createDummyGitArtifactBinary(): void { | |
| $bin_path = $this->getGitArtifactBinPath(); | |
| file_put_contents($bin_path, ''); | |
| $this->envSet('VORTEX_DEPLOY_ARTIFACT_GIT_ARTIFACT_SHA256', 'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855'); | |
| } | |
| /** | |
| * Mock the git-artifact download request. | |
| */ | |
| protected function mockGitArtifactDownload(): void { | |
| $this->mockRequest( | |
| 'https://github.com/drevops/git-artifact/releases/download/1.4.0/git-artifact', | |
| ['method' => 'GET'], | |
| ['ok' => TRUE, 'status' => 200, 'body' => ''], | |
| ); | |
| } | |
| protected const GIT_ARTIFACT_VERSION = '1.4.0'; | |
| protected function createDummyGitArtifactBinary(): void { | |
| $bin_path = $this->getGitArtifactBinPath(); | |
| file_put_contents($bin_path, ''); | |
| $this->envSet('VORTEX_DEPLOY_ARTIFACT_GIT_ARTIFACT_SHA256', 'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855'); | |
| } | |
| /** | |
| * Mock the git-artifact download request. | |
| */ | |
| protected function mockGitArtifactDownload(): void { | |
| $this->mockRequest( | |
| sprintf('https://github.com/drevops/git-artifact/releases/download/%s/git-artifact', self::GIT_ARTIFACT_VERSION), | |
| ['method' => 'GET'], | |
| ['ok' => TRUE, 'status' => 200, 'body' => ''], | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.vortex/tooling/tests/Unit/DeployArtifactTest.php around lines 395 - 410,
The mock in mockGitArtifactDownload() hardcodes the git-artifact version "1.4.0"
which can drift from the real default; extract that literal into a shared class
constant (e.g., DEFAULT_GIT_ARTIFACT_VERSION) and use that constant in
mockGitArtifactDownload() when building the download URL and anywhere else
(createDummyGitArtifactBinary or related tests) that needs the version so the
test remains in sync with the production default; alternatively read the default
from the same production constant/source and reference it in
mockGitArtifactDownload() and createDummyGitArtifactBinary().
…ault()' for re-read.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.vortex/tooling/src/provision:
- Around line 533-542: The log wording is inconsistent with the actual command:
when $provision_cache_rebuild_after_db_update_skip !== '1' the script runs
drush('cache:rebuild') but the messages use "Clearing cache" and "Cache was
cleared"; change those strings to "Rebuilding cache" and "Cache was rebuilt"
(matching the later block) so task(...), pass(...) messages accurately reflect
using drush('cache:rebuild') and remain consistent across the script.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5c4e2f59-60a2-4ea6-a0c7-269dcd272902
📒 Files selected for processing (2)
.vortex/tooling/src/download-db-container-registry.vortex/tooling/src/provision
Comment on lines
+533
to
+542
| if ($provision_cache_rebuild_after_db_update_skip !== '1') { | ||
| task('Clearing cache after database updates.'); | ||
| drush('cache:rebuild'); | ||
| pass('Cache was cleared.'); | ||
| echo PHP_EOL; | ||
| } | ||
| else { | ||
| pass('Skipped cache rebuild after database updates.'); | ||
| echo PHP_EOL; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider consistent terminology: "rebuild" vs "clear".
The message says "Clearing cache" (line 534) and "Cache was cleared" (line 536), but the command is drush('cache:rebuild'). Later in the script (lines 564-566), similar code says "Rebuilding cache" and "Cache was rebuilt" which is more accurate. Consider aligning the terminology for consistency.
✏️ Suggested terminology fix
if ($provision_cache_rebuild_after_db_update_skip !== '1') {
- task('Clearing cache after database updates.');
+ task('Rebuilding cache after database updates.');
drush('cache:rebuild');
- pass('Cache was cleared.');
+ pass('Cache was rebuilt.');
echo PHP_EOL;
}📝 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
| if ($provision_cache_rebuild_after_db_update_skip !== '1') { | |
| task('Clearing cache after database updates.'); | |
| drush('cache:rebuild'); | |
| pass('Cache was cleared.'); | |
| echo PHP_EOL; | |
| } | |
| else { | |
| pass('Skipped cache rebuild after database updates.'); | |
| echo PHP_EOL; | |
| } | |
| if ($provision_cache_rebuild_after_db_update_skip !== '1') { | |
| task('Rebuilding cache after database updates.'); | |
| drush('cache:rebuild'); | |
| pass('Cache was rebuilt.'); | |
| echo PHP_EOL; | |
| } | |
| else { | |
| pass('Skipped cache rebuild after database updates.'); | |
| echo PHP_EOL; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.vortex/tooling/src/provision around lines 533 - 542, The log wording is
inconsistent with the actual command: when
$provision_cache_rebuild_after_db_update_skip !== '1' the script runs
drush('cache:rebuild') but the messages use "Clearing cache" and "Cache was
cleared"; change those strings to "Rebuilding cache" and "Cache was rebuilt"
(matching the later block) so task(...), pass(...) messages accurately reflect
using drush('cache:rebuild') and remain consistent across the script.
|
Code coverage (threshold: 90%) Per-class coverage |
This comment has been minimized.
This comment has been minimized.
2 similar comments
This comment has been minimized.
This comment has been minimized.
Member
Author
|
Code coverage (threshold: 90%) Per-class coverage |
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
Ported recent shell script changes to the PHP tooling package, introducing multi-database index support across all download-db scripts, replacing Composer-based git-artifact installation with a direct binary download with SHA256 verification, adding a cache rebuild step after database updates in provision, and fixing the fallback profile installation to skip config import and enable Shield for protection.
Changes
Multi-database index support (
download-db,download-db-acquia,download-db-container-registry,download-db-ftp,download-db-lagoon,download-db-s3,download-db-url)VORTEX_DB_INDEXfrom the environment (defaults to empty string).VORTEX_DOWNLOAD_DB2_SOURCE) before falling back to the non-indexed variants, enabling multi-database setups.container_registrysource now skips the file existence check since the database is stored as a Docker image, not a file.download-db-lagoongained an explicitVORTEX_DOWNLOAD_DB_SSH_FINGERPRINTvariable (previously absent).git-artifact binary download (
deploy-artifact)composer global require drevops/git-artifact:~1.2with a direct binary download from GitHub Releases.VORTEX_DEPLOY_ARTIFACT_GIT_ARTIFACT_VERSION(default1.4.0) andVORTEX_DEPLOY_ARTIFACT_GIT_ARTIFACT_SHA256env vars.request()to$TMPDIR/git-artifact, verified withhash_file('sha256'), and made executable.curlprerequisite check was added before the download attempt.Provision improvements (
provision)VORTEX_PROVISION_CACHE_REBUILD_AFTER_DB_UPDATE_SKIPflag; when unset adrush cache:rebuildis now executed immediately afterdrush updb.$is_fallback = TRUEtoprovision_from_profile(), which:--existing-configfromdrush site:install.VORTEX_PROVISION_POST_OPERATIONS_SKIP=1viaputenv()to skip all post-provision operations.provision_from_profile()returns to pick up theputenv()change.notifyin_array($event, [...])now usesTRUEas the third argument.Tests (
DeployArtifactTest,ProvisionTest)DeployArtifactTest: replaced Composer install mocks withmockGitArtifactDownload()/mockRequest()helpers; addedtestGitArtifactDownloadFailureandtestGitArtifactSha256Failuretest cases; pre-creates dummy binary insetUp()with matching empty-file SHA256.ProvisionTest: updated all test cases to expect the new post-updbcache:rebuild step; addeddrush pm:install shieldexpectation and adjusted fallback tests to reflect skipped post-provision operations.Before / After
git-artifact installation
Provision fallback flow
Provision cache rebuild
Multi-database index variable resolution
Summary by CodeRabbit
New Features
Improvements