Improved Vortex update script to suport local and remote URL. #1840
Conversation
WalkthroughThis update standardizes the environment variable name for the Vortex template repository across scripts, documentation, and tests, changing it from Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant update-vortex.sh
participant LocalInstaller
participant RemoteInstaller
participant PHP
User->>update-vortex.sh: Run update-vortex.sh [args]
update-vortex.sh->>update-vortex.sh: Check for php and curl commands
alt VORTEX_INSTALLER_PATH is set and file exists
update-vortex.sh->>LocalInstaller: Use local installer script
else VORTEX_INSTALLER_PATH not set
update-vortex.sh->>RemoteInstaller: Download installer script (with cache bust)
RemoteInstaller-->>update-vortex.sh: Return installer.php
else VORTEX_INSTALLER_PATH set but missing
update-vortex.sh-->>User: Fail with error message
end
update-vortex.sh->>PHP: Execute installer.php with --uri and --no-interaction
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 phpcs (3.7.2).vortex/installer/src/Utils/Config.phpERROR: Referenced sniff "Drupal" does not exist Run "phpcs --help" for usage information .vortex/installer/src/Utils/Downloader.phpERROR: Referenced sniff "Drupal" does not exist Run "phpcs --help" for usage information .vortex/installer/tests/Unit/DownloaderTest.phpERROR: Referenced sniff "Drupal" does not exist Run "phpcs --help" for usage information
✨ 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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
AlexSkrypnyk
changed the title
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (22)
.vortex/installer/tests/Fixtures/install/_baseline/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/.github/workflows/assign-author.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/.github/workflows/draft-release-notes.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/.github/workflows/update-dependencies.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/.ignorecontentis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/composer.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/package.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/web/themes/custom/star_wars/yarn.lockis excluded by!**/yarn.lock,!**/*.lock,!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/yarn.lockis excluded by!**/yarn.lock,!**/*.lock,!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/ciprovider_circleci/.circleci/config.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/deploy_type_all_circleci/.circleci/config.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/deploy_type_none_circleci/.circleci/config.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/deploy_type_none_gha/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/deps_updates_provider_ci_circleci/.circleci/config.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/profile_the_empire/web/profiles/custom/the_empire/the_empire.info.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/provision_profile/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/services_no_clamav/composer.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/services_none/composer.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/timezone_circleci/.circleci/config.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/version_replacement/expected/composer.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/version_replacement/expected/update-dependencies.ymlis excluded by!.vortex/installer/tests/Fixtures/**
📒 Files selected for processing (13)
.ahoy.yml(1 hunks).vortex/docs/content/contributing/maintenance/tests.mdx(2 hunks).vortex/docs/content/workflows/variables.mdx(1 hunks).vortex/installer/src/Utils/Config.php(1 hunks).vortex/installer/src/Utils/Downloader.php(1 hunks).vortex/installer/tests/Unit/DownloaderTest.php(1 hunks).vortex/installer/tests/Unit/UnitTestCase.php(3 hunks).vortex/tests/bats/_helper.bash(1 hunks).vortex/tests/bats/e2e/update-vortex.bats(3 hunks).vortex/tests/bats/unit/update-vortex.bats(1 hunks).vortex/tests/phpunit/Traits/Steps/StepPrepareSutTrait.php(1 hunks).vortex/tests/test.common.sh(1 hunks)scripts/vortex/update-vortex.sh(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: Vortex - Test docs
.vortex/docs/content/workflows/variables.mdx
[error] 1139-1160: Documentation is not up-to-date. Run 'cd .vortex && ahoy update-docs' and commit changes.
⏰ 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 (0)
- GitHub Check: build (1)
- GitHub Check: vortex-test-installer (8.3)
- GitHub Check: vortex-test-installer (8.4)
- GitHub Check: vortex-test-installer (8.2)
- GitHub Check: vortex-test-workflow (2)
- GitHub Check: vortex-test-workflow (1)
- GitHub Check: vortex-test-common
- GitHub Check: vortex-test-deployment (1)
- GitHub Check: vortex-test-deployment (0)
- GitHub Check: vortex-test-workflow (3)
- GitHub Check: vortex-test-workflow (0)
🔇 Additional comments (29)
.vortex/installer/src/Utils/Downloader.php (1)
48-48: LGTM! Error message correction aligns with URI context.The change correctly updates the exception message from "Invalid remote repository format" to "Invalid local repository format" for
file://URIs, which accurately reflects the local repository context..vortex/tests/phpunit/Traits/Steps/StepPrepareSutTrait.php (1)
53-53: LGTM! Environment variable rename improves consistency.The change from
VORTEX_INSTALL_REPOtoVORTEX_INSTALL_TEMPLATE_REPOaligns with the broader standardization effort across the codebase and provides a more descriptive variable name..vortex/docs/content/workflows/variables.mdx (2)
1150-1150: LGTM! Variable name standardization is consistent.The rename from
VORTEX_INSTALL_REPOtoVORTEX_INSTALL_TEMPLATE_REPOmaintains consistency with the broader naming standardization effort across the codebase.
1150-1160: Address the pipeline failure for documentation updates.The GitHub Actions pipeline indicates the documentation is not up-to-date. Please run the documentation update command as suggested:
cd .vortex && ahoy update-docsThen commit the changes to resolve the pipeline failure.
.vortex/docs/content/contributing/maintenance/tests.mdx (1)
125-125: LGTM! Documentation examples updated consistently.Both instances correctly update the environment variable from
VORTEX_INSTALL_REPOtoVORTEX_INSTALL_TEMPLATE_REPO, maintaining consistency across the documentation and ensuring the examples remain functional.Also applies to: 163-163
.vortex/installer/src/Utils/Config.php (1)
22-24: LGTM! Constants updated for improved naming consistency.The updates to both
REPOandREFconstants fromVORTEX_INSTALL_*toVORTEX_INSTALL_TEMPLATE_*provide clearer, more descriptive names and align with the standardization effort across the codebase..ahoy.yml (1)
270-271: LGTM! Clear description and simplified command execution.The updated usage description better reflects the command's purpose. The removal of the confirmation prompt streamlines the workflow, though users should be aware that the command will now run without confirmation.
.vortex/tests/bats/_helper.bash (1)
1088-1088: LGTM! Environment variable renamed consistently.The renaming from
VORTEX_INSTALL_REPOtoVORTEX_INSTALL_TEMPLATE_REPOaligns with the PR's objective to standardize naming across the codebase..vortex/tests/test.common.sh (1)
44-48: LGTM! Enhanced test coverage with new test files.The addition of
login-container-registry.bats,setup-ssh.bats, andupdate-vortex.batstests aligns with the PR's enhancements. The reordering ofprovision.batsand addition of end-to-end tests for update-vortex functionality ensures comprehensive coverage.Also applies to: 52-52
.vortex/tests/bats/e2e/update-vortex.bats (4)
10-11: Excellent test readability improvements with substep annotations.The addition of descriptive
substepcalls throughout the test significantly improves readability and makes the test flow easier to follow during debugging.Also applies to: 16-17, 28-29, 39-40, 51-55, 61-62, 70-71, 74-75
48-48: LGTM! Environment variable renamed consistently.The change from
VORTEX_INSTALL_REFtoVORTEX_INSTALL_TEMPLATE_REFmaintains consistency with the broader renaming effort.
52-53: Good practice: Suppressing composer output in tests.Redirecting composer output to
/dev/nullreduces test noise while maintaining error handling through the exit codes.
77-78: Good addition: Verifying installer cleanup.The assertion that
installer.phpis removed after the update ensures proper cleanup behavior..vortex/installer/tests/Unit/UnitTestCase.php (1)
49-61: Well-structured refactoring with async file processing.The refactoring to use
File::replaceContentAsyncimproves performance for large directories. The file extension filtering (json, yml, yaml, dockerfile) appropriately targets files that typically contain version information..vortex/tests/bats/unit/update-vortex.bats (8)
11-34: Excellent test coverage for default behavior.This test properly validates the environment variable loading and default value handling. The use of a fixed cache bust value ensures deterministic test results.
36-58: Good test coverage for custom repository URI handling.The test correctly validates that custom template repository URIs passed as arguments are properly handled by the script.
60-87: Well-implemented test for local installer path functionality.The test properly creates a temporary installer file, validates the local path usage, and includes proper cleanup. Good test isolation practices.
89-109: Excellent error handling test.This test properly validates that the script fails gracefully when a specified local installer path doesn't exist, with appropriate error messaging.
111-133: Good coverage for file:// URL handling.The test correctly validates that file:// URLs for template repositories are properly handled and passed to the installer.
135-157: Appropriate test for local path handling.The test properly validates that local paths for template repositories are correctly processed and passed to the installer.
159-181: Comprehensive test for Git SSH URL support.The test correctly validates that Git SSH URLs with version references are properly handled by the script.
183-205: Critical test for error propagation.This test ensures that PHP installer execution failures are properly propagated, which is essential for CI/CD pipeline failure detection and automated deployment reliability.
scripts/vortex/update-vortex.sh (4)
3-29: Excellent documentation improvements and clear variable naming.The updated script purpose description and comprehensive URI format examples greatly improve usability. The environment variable rename to
VORTEX_INSTALL_TEMPLATE_REPOmakes the purpose much clearer.
34-40: Well-designed new environment variables.The addition of cache busting and local installer path support provides excellent flexibility for both production use (avoiding stale downloads) and development scenarios (using local installer versions).
51-55: Good defensive programming with pre-flight checks.The upfront validation of required commands (
php,curl) provides clear error messages and prevents confusing failures later in the script execution.
57-74: Excellent implementation of local vs remote installer logic.The conditional logic properly handles both local and remote installer scenarios with comprehensive error handling. The cache busting implementation and clear error messages enhance reliability and debuggability.
.vortex/installer/tests/Unit/DownloaderTest.php (3)
11-28: Well-structured test class with proper PHPUnit practices.The test method implementation correctly handles both success and exception cases, with proper type declarations and clear assertion logic for validating parsed repository and reference components.
31-237: Excellent comprehensive coverage of valid URI formats.The data provider includes thorough test cases for all supported URI formats (HTTPS, Git SSH, file://, local paths) with various reference types and edge cases. The coverage ensures robust validation of the parsing logic.
239-427: Comprehensive error case validation with clear error messages.The invalid test cases thoroughly validate error handling for malformed URIs and references, with specific and helpful error messages that will aid in debugging. The coverage includes edge cases and boundary conditions.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1840 +/- ##
===========================================
+ Coverage 74.09% 75.01% +0.91%
===========================================
Files 84 84
Lines 4830 4839 +9
Branches 35 35
===========================================
+ Hits 3579 3630 +51
+ Misses 1251 1209 -42 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
AlexSkrypnyk
force-pushed
the
feature/better-vortex-update
branch
from
c073cb2 to
79d70d1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.vortex/installer/tests/Unit/UnitTestCase.php (1)
98-100: Consider documenting the early exit logic.The early exit after 4 pattern replacements appears to be an optimization, but the rationale for the specific threshold of 4 is unclear. Consider adding a comment explaining why this limit was chosen.
+ // Early exit after 4 replacements to prevent over-processing of files + // that match too many patterns (likely not version-related content). if ($replaced > 4) { break; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (22)
.vortex/installer/tests/Fixtures/install/_baseline/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/.github/workflows/assign-author.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/.github/workflows/draft-release-notes.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/.github/workflows/update-dependencies.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/.ignorecontentis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/composer.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/package.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/web/themes/custom/star_wars/yarn.lockis excluded by!**/yarn.lock,!**/*.lock,!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/yarn.lockis excluded by!**/yarn.lock,!**/*.lock,!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/ciprovider_circleci/.circleci/config.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/deploy_type_all_circleci/.circleci/config.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/deploy_type_none_circleci/.circleci/config.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/deploy_type_none_gha/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/deps_updates_provider_ci_circleci/.circleci/config.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/profile_the_empire/web/profiles/custom/the_empire/the_empire.info.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/provision_profile/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/services_no_clamav/composer.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/services_none/composer.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/timezone_circleci/.circleci/config.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/version_replacement/expected/composer.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/version_replacement/expected/update-dependencies.ymlis excluded by!.vortex/installer/tests/Fixtures/**
📒 Files selected for processing (16)
.ahoy.yml(1 hunks).vortex/docs/content/contributing/maintenance/tests.mdx(2 hunks).vortex/docs/content/workflows/variables.mdx(2 hunks).vortex/installer/src/Utils/Config.php(1 hunks).vortex/installer/src/Utils/Downloader.php(1 hunks).vortex/installer/tests/Unit/DownloaderTest.php(1 hunks).vortex/installer/tests/Unit/UnitTestCase.php(3 hunks).vortex/tests/bats/_helper.bash(1 hunks).vortex/tests/bats/_helper.deployment.bash(1 hunks).vortex/tests/bats/e2e/update-vortex.bats(3 hunks).vortex/tests/bats/unit/deploy-container-registry.bats(2 hunks).vortex/tests/bats/unit/login-container-registry.bats(1 hunks).vortex/tests/bats/unit/update-vortex.bats(1 hunks).vortex/tests/phpunit/Traits/Steps/StepPrepareSutTrait.php(1 hunks).vortex/tests/test.common.sh(1 hunks)scripts/vortex/update-vortex.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). (13)
- GitHub Check: build (1)
- GitHub Check: build (0)
- GitHub Check: vortex-test-workflow (0)
- GitHub Check: vortex-test-deployment (1)
- GitHub Check: vortex-test-workflow (3)
- GitHub Check: vortex-test-deployment (0)
- GitHub Check: vortex-test-workflow (2)
- GitHub Check: vortex-test-workflow (1)
- GitHub Check: vortex-test-common
- GitHub Check: vortex-test-installer (8.2)
- GitHub Check: vortex-test-installer (8.4)
- GitHub Check: vortex-test-installer (8.3)
- GitHub Check: vortex-test-docs
🔇 Additional comments (25)
.vortex/installer/src/Utils/Config.php (1)
22-24: LGTM! Environment variable constant names improved for clarity.The renaming from
VORTEX_INSTALL_REPOtoVORTEX_INSTALL_TEMPLATE_REPOandVORTEX_INSTALL_REFtoVORTEX_INSTALL_TEMPLATE_REFmakes the purpose of these constants clearer - they specifically refer to template repository configuration..vortex/tests/bats/_helper.deployment.bash (1)
164-168: Excellent improvement to Docker config test helper.The function rename from
provision_docker_config_filetocreate_docker_config_fileis more descriptive, and the implementation now creates a proper JSON Docker authentication config with base64-encoded credentials instead of a simple string. This makes the test helper more realistic and accurate for testing Docker registry interactions..vortex/tests/phpunit/Traits/Steps/StepPrepareSutTrait.php (1)
53-53: LGTM! Environment variable name updated for consistency.The change from
VORTEX_INSTALL_REPOtoVORTEX_INSTALL_TEMPLATE_REPOmaintains consistency with the environment variable standardization across the codebase while preserving the same functionality..vortex/installer/src/Utils/Downloader.php (1)
48-48: Good fix! Error message now accurately reflects the URI type being validated.The error message was corrected from "Invalid remote repository format" to "Invalid local repository format" which is accurate since this code path handles
file://URIs that represent local repositories. This would have been confusing for users encountering parsing errors with local file paths..vortex/tests/bats/unit/deploy-container-registry.bats (1)
36-36: LGTM! Function calls updated to match the renamed helper.The calls to
create_docker_config_fileare correctly updated from the oldprovision_docker_config_filename, and now properly pass the registry parameter that the improved implementation expects.Also applies to: 90-90
.vortex/tests/bats/unit/login-container-registry.bats (1)
38-38: LGTM! Helper function call updated correctly.The call to
create_docker_config_filealigns with the helper function renaming mentioned in the AI summary..vortex/docs/content/contributing/maintenance/tests.mdx (1)
125-125: LGTM! Environment variable standardization applied correctly.The documentation updates consistently rename
VORTEX_INSTALL_REPOtoVORTEX_INSTALL_TEMPLATE_REPOin both code examples, aligning with the broader standardization effort across the codebase.Also applies to: 163-163
.vortex/tests/test.common.sh (1)
44-48: LGTM! Test coverage enhanced appropriately.The addition of new unit tests (
login-container-registry.bats,setup-ssh.bats,update-vortex.bats) and the end-to-end test corresponds well with the functionality being enhanced in this PR. The test reordering maintains logical execution flow.Also applies to: 52-52
.ahoy.yml (1)
270-271: LGTM! Command interface improved for better user experience.The updated usage description is more accurate and descriptive. Removing the interactive confirmation simplifies the workflow while the underlying script can handle any necessary safeguards internally.
.vortex/tests/bats/_helper.bash (1)
1088-1088: LGTM! Environment variable rename consistent with standardization effort.The change from
VORTEX_INSTALL_REPOtoVORTEX_INSTALL_TEMPLATE_REPOmaintains the same functionality while aligning with the broader environment variable standardization across the codebase..vortex/tests/bats/e2e/update-vortex.bats (4)
10-10: Good addition of substep annotations for improved test clarity.The substep annotations make the test flow more readable and easier to debug when failures occur.
Also applies to: 16-16, 21-21, 28-28, 39-39, 51-51, 57-57, 61-61, 70-70, 74-74, 77-77
48-48: Correct environment variable rename aligned with codebase standardization.The rename from
VORTEX_INSTALL_REFtoVORTEX_INSTALL_TEMPLATE_REFis consistent with the broader effort to standardize environment variable naming.
52-53: Appropriate output suppression for cleaner test logs.Redirecting composer output to
/dev/nullreduces noise in test output while still allowing error codes to be checked.
55-55: Valuable additions for debugging and cleanup verification.Displaying the installer version aids in debugging, and asserting the removal of
installer.phpensures proper cleanup after the update process.Also applies to: 77-78
.vortex/docs/content/workflows/variables.mdx (1)
1142-1173: Well-documented environment variables with comprehensive examples.The new environment variables are clearly documented with appropriate descriptions, default values, and comprehensive examples covering all supported repository URI formats.
.vortex/installer/tests/Unit/UnitTestCase.php (2)
49-61: Good refactoring to use async file processing.The change to
File::replaceContentAsyncimproves performance when processing multiple files, and the file extension filtering is appropriate for version replacement operations.
63-104: Clean extraction of replacement logic.The
replaceVersionsInLinemethod cleanly separates the replacement logic, making the code more modular and testable. The comprehensive patterns cover various version formats effectively..vortex/tests/bats/unit/update-vortex.bats (1)
1-206: Comprehensive and well-structured unit tests.The unit tests provide excellent coverage of the update-vortex.sh script, including:
- Default behavior and environment variable handling
- Local installer path functionality
- Various repository URI formats
- Proper error handling for missing files and command failures
The mocking approach and test structure follow BATS best practices.
scripts/vortex/update-vortex.sh (4)
14-29: Excellent documentation of supported repository URI formats.The comprehensive examples clearly demonstrate all supported URI formats including remote repositories, local repositories, file:// URIs, and various reference types (tags, commits).
34-40: Good additions for cache busting and local installer support.The cache busting parameter helps avoid stale installer downloads, and the local installer path option provides valuable flexibility for development and testing scenarios.
52-55: Proper command availability checks with correct quoting.The pre-flight checks ensure required commands are available before proceeding, with proper quoting to prevent word splitting issues.
57-72: Well-structured conditional logic for installer sourcing.The implementation correctly handles both local and remote installer scenarios with appropriate error handling and user feedback. The prioritization of local installer path over URL download is logical.
.vortex/installer/tests/Unit/DownloaderTest.php (3)
1-12: Excellent modern PHP and PHPUnit setup.The file structure follows best practices with strict typing, proper namespacing, and modern PHP 8+ attributes. The
#[CoversClass]attribute correctly specifies the class under test.
14-28: Well-structured test method with proper exception handling.The test method correctly handles both success and exception scenarios. The conditional logic properly sets up exception expectations before calling the method under test, and includes descriptive assertion messages for better debugging.
239-427: Comprehensive invalid test case coverage with one potential issue.The invalid test cases provide excellent coverage of malformed inputs, validation edge cases, and specific error scenarios. The exception messages are descriptive and helpful for debugging.
However, there's a potential issue with the test case on lines 233-237:
'Empty reference defaults to HEAD - @ is captured in repo part' => [ '/path/to/repo@', '/path/to/repo@', Downloader::REF_HEAD, ],This suggests that
'/path/to/repo@'(with trailing@but no reference) results in the repo being'/path/to/repo@'rather than'/path/to/repo'. This seems like unexpected behavior - a trailing@with no reference should either be treated as an error or the@should be stripped from the repo part.Please verify if this is the intended behavior or if this test case should be moved to the invalid test cases section.
| public static function dataProviderParseUri(): array { | ||
| return [ | ||
| // Valid test cases. | ||
| 'HTTPS URLs - with default HEAD reference - GitHub' => [ | ||
| 'https://github.com/user/repo', | ||
| 'https://github.com/user/repo', | ||
| Downloader::REF_HEAD, | ||
| ], | ||
| 'HTTPS URLs - with default HEAD reference - GitLab' => [ | ||
| 'https://gitlab.com/user/repo', | ||
| 'https://gitlab.com/user/repo', | ||
| Downloader::REF_HEAD, | ||
| ], | ||
| 'HTTPS URLs - with default HEAD reference - Bitbucket' => [ | ||
| 'https://bitbucket.org/user/repo', | ||
| 'https://bitbucket.org/user/repo', | ||
| Downloader::REF_HEAD, | ||
| ], | ||
| 'HTTPS URLs - with specific valid references - stable' => [ | ||
| 'https://github.com/user/repo@stable', | ||
| 'https://github.com/user/repo', | ||
| Downloader::REF_STABLE, | ||
| ], | ||
| 'HTTPS URLs - with specific valid references - HEAD' => [ | ||
| 'https://github.com/user/repo@HEAD', | ||
| 'https://github.com/user/repo', | ||
| Downloader::REF_HEAD, | ||
| ], | ||
| 'HTTPS URLs - with 40-character commit hash' => [ | ||
| 'https://github.com/user/repo@1234567890abcdef1234567890abcdef12345678', | ||
| 'https://github.com/user/repo', | ||
| '1234567890abcdef1234567890abcdef12345678', | ||
| ], | ||
| 'HTTPS URLs - with 7-character short commit hash' => [ | ||
| 'https://github.com/user/repo@1234567', | ||
| 'https://github.com/user/repo', | ||
| '1234567', | ||
| ], | ||
| 'Git SSH URLs - with default HEAD reference - GitHub' => [ | ||
| 'git@github.com:user/repo', | ||
| 'git@github.com:user/repo', | ||
| Downloader::REF_HEAD, | ||
| ], | ||
| 'Git SSH URLs - with default HEAD reference - GitLab' => [ | ||
| 'git@gitlab.com:user/repo', | ||
| 'git@gitlab.com:user/repo', | ||
| Downloader::REF_HEAD, | ||
| ], | ||
| 'Git SSH URLs - with default HEAD reference - Bitbucket' => [ | ||
| 'git@bitbucket.org:user/repo', | ||
| 'git@bitbucket.org:user/repo', | ||
| Downloader::REF_HEAD, | ||
| ], | ||
| 'Git SSH URLs - with specific valid references - stable' => [ | ||
| 'git@github.com:user/repo@stable', | ||
| 'git@github.com:user/repo', | ||
| Downloader::REF_STABLE, | ||
| ], | ||
| 'Git SSH URLs - with specific valid references - HEAD' => [ | ||
| 'git@github.com:user/repo@HEAD', | ||
| 'git@github.com:user/repo', | ||
| Downloader::REF_HEAD, | ||
| ], | ||
| 'Git SSH URLs - with commit hashes - 40 char' => [ | ||
| 'git@github.com:user/repo@1234567890abcdef1234567890abcdef12345678', | ||
| 'git@github.com:user/repo', | ||
| '1234567890abcdef1234567890abcdef12345678', | ||
| ], | ||
| 'Git SSH URLs - with commit hashes - 7 char' => [ | ||
| 'git@github.com:user/repo@1234567', | ||
| 'git@github.com:user/repo', | ||
| '1234567', | ||
| ], | ||
| 'File URLs - with default HEAD reference' => [ | ||
| 'file:///path/to/repo', | ||
| '/path/to/repo', | ||
| Downloader::REF_HEAD, | ||
| ], | ||
| 'File URLs - with default HEAD reference - user path' => [ | ||
| 'file:///home/user/repos/myrepo', | ||
| '/home/user/repos/myrepo', | ||
| Downloader::REF_HEAD, | ||
| ], | ||
| 'File URLs - with specific valid references - stable' => [ | ||
| 'file:///path/to/repo@stable', | ||
| '/path/to/repo', | ||
| Downloader::REF_STABLE, | ||
| ], | ||
| 'File URLs - with specific valid references - HEAD' => [ | ||
| 'file:///path/to/repo@HEAD', | ||
| '/path/to/repo', | ||
| Downloader::REF_HEAD, | ||
| ], | ||
| 'File URLs - with 40-character commit hash' => [ | ||
| 'file:///path/to/repo@1234567890abcdef1234567890abcdef12345678', | ||
| '/path/to/repo', | ||
| '1234567890abcdef1234567890abcdef12345678', | ||
| ], | ||
| 'File URLs - with 7-character commit hash' => [ | ||
| 'file:///path/to/repo@1234567', | ||
| '/path/to/repo', | ||
| '1234567', | ||
| ], | ||
| 'Local paths - with default HEAD reference - absolute' => [ | ||
| '/path/to/repo', | ||
| '/path/to/repo', | ||
| Downloader::REF_HEAD, | ||
| ], | ||
| 'Local paths - with default HEAD reference - user home' => [ | ||
| '/home/user/repos/myrepo', | ||
| '/home/user/repos/myrepo', | ||
| Downloader::REF_HEAD, | ||
| ], | ||
| 'Local paths - with default HEAD reference - relative' => [ | ||
| 'relative/path/to/repo', | ||
| 'relative/path/to/repo', | ||
| Downloader::REF_HEAD, | ||
| ], | ||
| 'Local paths - with default HEAD reference - current dir' => [ | ||
| './repo', | ||
| './repo', | ||
| Downloader::REF_HEAD, | ||
| ], | ||
| 'Local paths - with default HEAD reference - parent dir' => [ | ||
| '../repo', | ||
| '../repo', | ||
| Downloader::REF_HEAD, | ||
| ], | ||
| 'Local paths - with specific valid references - stable' => [ | ||
| '/path/to/repo@stable', | ||
| '/path/to/repo', | ||
| Downloader::REF_STABLE, | ||
| ], | ||
| 'Local paths - with specific valid references - HEAD' => [ | ||
| '/path/to/repo@HEAD', | ||
| '/path/to/repo', | ||
| Downloader::REF_HEAD, | ||
| ], | ||
| 'Local paths - with 40-character commit hash' => [ | ||
| '/path/to/repo@1234567890abcdef1234567890abcdef12345678', | ||
| '/path/to/repo', | ||
| '1234567890abcdef1234567890abcdef12345678', | ||
| ], | ||
| 'Local paths - with 7-character commit hash' => [ | ||
| '/path/to/repo@1234567', | ||
| '/path/to/repo', | ||
| '1234567', | ||
| ], | ||
| 'Local paths with trailing slashes - should be trimmed - single slash' => [ | ||
| '/path/to/repo/', | ||
| '/path/to/repo', | ||
| Downloader::REF_HEAD, | ||
| ], | ||
| 'Local paths with trailing slashes - should be trimmed - double slash' => [ | ||
| '/path/to/repo//', | ||
| '/path/to/repo', | ||
| Downloader::REF_HEAD, | ||
| ], | ||
| 'Local paths with trailing slashes - should be trimmed - with reference' => [ | ||
| '/path/to/repo/@stable', | ||
| '/path/to/repo', | ||
| Downloader::REF_STABLE, | ||
| ], | ||
| 'Relative paths - simple' => [ | ||
| 'repo', | ||
| 'repo', | ||
| Downloader::REF_HEAD, | ||
| ], | ||
| 'Relative paths - with reference' => [ | ||
| 'repo@stable', | ||
| 'repo', | ||
| Downloader::REF_STABLE, | ||
| ], | ||
| 'Edge cases with valid commit hashes - uppercase 40 char' => [ | ||
| 'https://github.com/user/repo@ABCDEF1234567890ABCDEF1234567890ABCDEF12', | ||
| 'https://github.com/user/repo', | ||
| 'ABCDEF1234567890ABCDEF1234567890ABCDEF12', | ||
| ], | ||
| 'Edge cases with valid commit hashes - uppercase 7 char' => [ | ||
| 'git@github.com:user/repo@ABCDEF1', | ||
| 'git@github.com:user/repo', | ||
| 'ABCDEF1', | ||
| ], | ||
| 'Edge cases that are actually valid - HTTPS with extra path' => [ | ||
| 'https://github.com/user/repo/extra/path', | ||
| 'https://github.com/user/repo/extra/path', | ||
| Downloader::REF_HEAD, | ||
| ], | ||
| 'Edge cases that are actually valid - Git SSH with extra path' => [ | ||
| 'git@github.com:user/repo/extra', | ||
| 'git@github.com:user/repo/extra', | ||
| Downloader::REF_HEAD, | ||
| ], | ||
| 'Edge cases that are actually valid - protocol-less GitHub' => [ | ||
| 'github.com/user/repo', | ||
| 'github.com/user/repo', | ||
| Downloader::REF_HEAD, | ||
| ], | ||
| 'Edge cases that are actually valid - file root' => [ | ||
| 'file:///', | ||
| '/', | ||
| Downloader::REF_HEAD, | ||
| ], | ||
| 'Empty reference defaults to HEAD - @ is captured in repo part' => [ | ||
| '/path/to/repo@', | ||
| '/path/to/repo@', | ||
| Downloader::REF_HEAD, | ||
| ], | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Exceptional test coverage with comprehensive scenarios.
The data provider demonstrates excellent test coverage across all supported URI formats (HTTPS, SSH, file URLs, local paths) with descriptive test case names. The coverage includes edge cases like trailing slashes, case variations, and different reference formats.
Consider grouping related test cases with comments or extracting into separate data provider methods for improved maintainability:
public static function dataProviderParseUri(): array {
return array_merge(
self::getValidHttpsTestCases(),
self::getValidSshTestCases(),
self::getValidFileUrlTestCases(),
self::getValidLocalPathTestCases(),
self::getInvalidTestCases()
);
}🤖 Prompt for AI Agents
In .vortex/installer/tests/Unit/DownloaderTest.php from lines 30 to 238, the
dataProviderParseUri method contains a large array of test cases for various URI
formats. To improve maintainability and readability, refactor this method by
grouping related test cases into separate private static methods (e.g.,
getValidHttpsTestCases, getValidSshTestCases, etc.) and then merge their results
in dataProviderParseUri using array_merge. This modular approach will keep the
test data organized and easier to manage.
b5cb2db to
d36f6ab
Compare
AlexSkrypnyk
force-pushed
the
feature/better-vortex-update
branch
from
d36f6ab to
a12ffe6
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
scripts/vortex/login-container-registry.sh (1)
55-62: Use fixed-string grep (or jq) to avoid false positives when checking login status
grep -q "${VORTEX_CONTAINER_REGISTRY}"treats the pattern as a regular expression, so dots and other meta-characters are interpreted.
Example: a registry calleddocker.iowill matchdockerXioand pass the check even though no valid auth exists.-if [ -f "${DOCKER_CONFIG}/config.json" ] && grep -q "${VORTEX_CONTAINER_REGISTRY}" "${DOCKER_CONFIG}/config.json"; then +if [ -f "${DOCKER_CONFIG}/config.json" ] && grep -qF "${VORTEX_CONTAINER_REGISTRY}" "${DOCKER_CONFIG}/config.json"; then
-Fmakes the match literal and avoids accidental regex matches.
(If you are comfortable depending onjq, a JSON-aware lookup would be even safer.)
♻️ Duplicate comments (1)
.vortex/installer/tests/Unit/DownloaderTest.php (1)
30-238: Comprehensive test coverage validates all URI parsing scenarios.The data provider covers all supported URI formats (HTTPS, SSH, file URLs, local paths) with excellent edge case coverage including trailing slashes, case variations, and different reference formats. This ensures robust validation of the URI parsing logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (22)
.vortex/installer/tests/Fixtures/install/_baseline/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/.github/workflows/assign-author.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/.github/workflows/draft-release-notes.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/.github/workflows/update-dependencies.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/.ignorecontentis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/composer.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/package.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/web/themes/custom/star_wars/yarn.lockis excluded by!**/yarn.lock,!**/*.lock,!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/yarn.lockis excluded by!**/yarn.lock,!**/*.lock,!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/ciprovider_circleci/.circleci/config.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/deploy_type_all_circleci/.circleci/config.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/deploy_type_none_circleci/.circleci/config.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/deploy_type_none_gha/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/deps_updates_provider_ci_circleci/.circleci/config.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/profile_the_empire/web/profiles/custom/the_empire/the_empire.info.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/provision_profile/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/services_no_clamav/composer.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/services_none/composer.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/timezone_circleci/.circleci/config.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/version_replacement/expected/composer.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/version_replacement/expected/update-dependencies.ymlis excluded by!.vortex/installer/tests/Fixtures/**
📒 Files selected for processing (17)
.ahoy.yml(1 hunks).vortex/docs/content/contributing/maintenance/tests.mdx(2 hunks).vortex/docs/content/workflows/variables.mdx(2 hunks).vortex/installer/src/Utils/Config.php(1 hunks).vortex/installer/src/Utils/Downloader.php(1 hunks).vortex/installer/tests/Unit/DownloaderTest.php(1 hunks).vortex/installer/tests/Unit/UnitTestCase.php(3 hunks).vortex/tests/bats/_helper.bash(1 hunks).vortex/tests/bats/_helper.deployment.bash(1 hunks).vortex/tests/bats/e2e/update-vortex.bats(3 hunks).vortex/tests/bats/unit/deploy-container-registry.bats(2 hunks).vortex/tests/bats/unit/login-container-registry.bats(2 hunks).vortex/tests/bats/unit/update-vortex.bats(1 hunks).vortex/tests/phpunit/Traits/Steps/StepPrepareSutTrait.php(1 hunks).vortex/tests/test.common.sh(1 hunks)scripts/vortex/login-container-registry.sh(1 hunks)scripts/vortex/update-vortex.sh(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: Vortex - Test installer
.vortex/installer/tests/Unit/UnitTestCase.php
[warning] 98-98: Line exceeds 80 characters; contains 82 characters (Drupal.Files.LineLength.TooLong)
[warning] 99-99: Line exceeds 80 characters; contains 81 characters (Drupal.Files.LineLength.TooLong)
[warning] 101-101: Line exceeds 80 characters; contains 81 characters (Drupal.Files.LineLength.TooLong)
⏰ 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). (9)
- GitHub Check: build (0)
- GitHub Check: build (1)
- GitHub Check: vortex-test-workflow (3)
- GitHub Check: vortex-test-workflow (1)
- GitHub Check: vortex-test-workflow (0)
- GitHub Check: vortex-test-deployment (0)
- GitHub Check: vortex-test-common
- GitHub Check: vortex-test-deployment (1)
- GitHub Check: vortex-test-workflow (2)
🔇 Additional comments (24)
.vortex/tests/bats/_helper.deployment.bash (1)
164-168: LGTM! Improved function naming and implementation.The function rename from
provision_docker_config_filetocreate_docker_config_fileis more descriptive, and the implementation now correctly writes a proper JSON Docker authentication config file format instead of a simple string..vortex/installer/src/Utils/Downloader.php (1)
48-48: LGTM! Corrected error message for local repository format.The exception message now correctly indicates "Invalid local repository format" instead of "Invalid remote repository format" for
file://URI validation, which accurately reflects the context..vortex/tests/phpunit/Traits/Steps/StepPrepareSutTrait.php (1)
53-53: LGTM! Environment variable renamed for clarity.The change from
VORTEX_INSTALL_REPOtoVORTEX_INSTALL_TEMPLATE_REPOimproves clarity by explicitly indicating this variable is for the template repository path..vortex/tests/bats/unit/deploy-container-registry.bats (2)
36-36: LGTM! Updated to use renamed helper function.The function call correctly updated from
provision_docker_config_filetocreate_docker_config_fileto match the renamed helper function.
90-90: LGTM! Consistent function name update.Another instance of the function call correctly updated to use the renamed helper function.
.vortex/docs/content/contributing/maintenance/tests.mdx (2)
125-125: LGTM! Documentation updated for renamed environment variable.The documentation correctly reflects the environment variable rename from
VORTEX_INSTALL_REPOtoVORTEX_INSTALL_TEMPLATE_REPO, ensuring consistency with the codebase changes.
163-163: LGTM! Consistent documentation update.Another instance of the documentation correctly updated to use the renamed environment variable.
.vortex/tests/bats/_helper.bash (1)
1084-1089: LGTM – variable rename reflected in test helperThe export now uses
VORTEX_INSTALL_TEMPLATE_REPO, matching the new installer convention..vortex/tests/test.common.sh (1)
44-49: Tests order update looks fineAdditional unit suites are wired into the pipeline without altering runner logic.
.vortex/tests/bats/e2e/update-vortex.bats (1)
10-74: LGTM! Good test organization with substep annotations.The addition of
substepannotations improves test readability and traceability. The changes align well with the broader refactoring of environment variables..vortex/tests/bats/unit/login-container-registry.bats (2)
28-44: LGTM! Improved test clarity and structure.The test name change better describes the scenario being tested, and the explicit environment variable setting makes the test more self-documenting.
73-81: LGTM! Better test structure with step-based mocking.The transition to step-based mocking improves test maintainability and readability. This pattern aligns well with other tests in the suite.
.vortex/docs/content/workflows/variables.mdx (2)
1142-1148: LGTM! Clear documentation for the new installer path variable.The documentation clearly explains the purpose and behavior of
VORTEX_INSTALLER_PATH, including its precedence overVORTEX_INSTALLER_URL.
1166-1172: Excellent documentation with comprehensive examples.The documentation for
VORTEX_INSTALL_TEMPLATE_REPOis thorough with examples covering all supported URI formats. The rename fromVORTEX_INSTALL_REPOmakes the variable's purpose clearer..vortex/installer/tests/Unit/UnitTestCase.php (1)
48-61: LGTM! Good refactoring with async operations.The refactoring to use
File::replaceContentAsyncwith file extension filtering improves performance and maintainability. The separation of concerns between file processing and content replacement is well done..vortex/tests/bats/unit/update-vortex.bats (1)
1-205: LGTM! Comprehensive test coverage for the update script.This test suite provides excellent coverage of the
update-vortex.shscript functionality, including:
- Default behavior and environment variable handling
- Support for various repository URI formats
- Local installer path functionality
- Proper error handling for missing files and execution failures
The step-based mocking approach is consistent and maintainable.
scripts/vortex/update-vortex.sh (6)
14-29: Excellent documentation and URI format support.The comprehensive examples clearly document all supported URI formats including HTTPS, SSH, file URLs, and local paths with various reference types. This greatly improves usability and reduces potential confusion.
29-29: Environment variable renamed consistently.The rename from
VORTEX_INSTALL_REPOtoVORTEX_INSTALL_TEMPLATE_REPOimproves clarity and aligns with the PR objectives.
34-39: Good addition of cache busting and local installer override.The cache busting parameter ensures fresh downloads, and the local installer path override provides flexibility for development and testing scenarios.
51-55: Essential pre-flight checks implemented correctly.Checking for required commands (
phpandcurl) before execution prevents runtime failures with clear error messages.
57-71: Robust installer path handling with proper error checking.The conditional logic correctly handles both local and remote installer scenarios with appropriate error handling for missing local files and download failures.
73-73: Clean execution with proper parameter passing.The final execution passes the renamed environment variable correctly to the PHP installer with appropriate flags.
.vortex/installer/tests/Unit/DownloaderTest.php (2)
1-28: Well-structured test class with proper PHPUnit attributes.The test class follows modern PHPUnit conventions with proper attributes, type hints, and exception handling logic. The test method correctly validates both successful parsing and exception scenarios.
239-426: Thorough validation of error conditions and edge cases.The invalid test cases comprehensively cover malformed URIs, invalid reference formats, and edge cases with proper exception message validation. This ensures the parser correctly rejects invalid input with meaningful error messages.
| usage: Update project from the Vortex template repository. | ||
| cmd: ./scripts/vortex/update-vortex.sh $@ | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Quote $@ to preserve spaces in forwarded arguments
When users pass options containing spaces, an unquoted $@ will lose grouping.
- cmd: ./scripts/vortex/update-vortex.sh $@
+ cmd: ./scripts/vortex/update-vortex.sh "$@"Small change, big UX win.
📝 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: Update project from the Vortex template repository. | |
| cmd: ./scripts/vortex/update-vortex.sh $@ | |
| usage: Update project from the Vortex template repository. | |
| cmd: ./scripts/vortex/update-vortex.sh "$@" |
🤖 Prompt for AI Agents
In the .ahoy.yml file around lines 270 to 272, the usage of $@ in the cmd line
is unquoted, which causes arguments with spaces to be split incorrectly. To fix
this, quote "$@" in the cmd value to preserve spaces in forwarded arguments,
changing it to ./scripts/vortex/update-vortex.sh "$@".
| const REPO = 'VORTEX_INSTALL_TEMPLATE_REPO'; | ||
|
|
||
| const REF = 'VORTEX_INSTALL_REF'; | ||
| const REF = 'VORTEX_INSTALL_TEMPLATE_REF'; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Provide backward-compatibility shim for the old environment variables
Renaming the variables is fine, but installations that still export VORTEX_INSTALL_REPO / VORTEX_INSTALL_REF will silently break.
Consider falling back to the legacy names if the new ones are absent:
// after constructor or in a bootstrap method
+// Back-compat: honour old env-vars if new ones are not set
+if (!Env::get(self::REPO) && Env::get('VORTEX_INSTALL_REPO')) {
+ $this->set(self::REPO, Env::get('VORTEX_INSTALL_REPO'), true);
+}
+if (!Env::get(self::REF) && Env::get('VORTEX_INSTALL_REF')) {
+ $this->set(self::REF, Env::get('VORTEX_INSTALL_REF'), true);
+}This prevents unexpected failures after upgrading.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In .vortex/installer/src/Utils/Config.php around lines 22 to 25, the new
environment variable constants REPO and REF should support backward
compatibility by falling back to the old environment variables
VORTEX_INSTALL_REPO and VORTEX_INSTALL_REF if the new ones are not set. Modify
the code to check if the new environment variables exist and use them;
otherwise, use the legacy environment variables to prevent breaking existing
installations.
| composer --working-dir="${ROOT_DIR}/.vortex/installer" install >/dev/null | ||
| composer --working-dir="${ROOT_DIR}/.vortex/installer" build >/dev/null |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider capturing composer output for debugging.
While redirecting composer output to /dev/null reduces noise, it might make debugging failures more difficult. Consider capturing the output in a variable or log file for potential debugging needs.
- composer --working-dir="${ROOT_DIR}/.vortex/installer" install >/dev/null
- composer --working-dir="${ROOT_DIR}/.vortex/installer" build >/dev/null
+ composer --working-dir="${ROOT_DIR}/.vortex/installer" install >"${TEST_LOG_DIR}/composer-install.log" 2>&1
+ composer --working-dir="${ROOT_DIR}/.vortex/installer" build >"${TEST_LOG_DIR}/composer-build.log" 2>&1🤖 Prompt for AI Agents
In .vortex/tests/bats/e2e/update-vortex.bats around lines 52 to 53, the composer
commands redirect all output to /dev/null, which hides useful debugging
information. Modify the commands to capture their output into variables or
redirect output to log files instead of /dev/null, so that the output can be
reviewed if the commands fail during testing.
| @test "Local installer path is used when VORTEX_INSTALLER_PATH is set and file exists" { | ||
| pushd "${LOCAL_REPO_DIR}" >/dev/null || exit 1 | ||
|
|
||
| create_global_command_wrapper "curl" | ||
| create_global_command_wrapper "php" | ||
|
|
||
| # Create temporary installer file. | ||
| local test_installer="${LOCAL_REPO_DIR}/test-installer.php" | ||
| echo "<?php echo 'test installer';" > "${test_installer}" | ||
|
|
||
| export VORTEX_INSTALLER_PATH="${test_installer}" | ||
|
|
||
| declare -a STEPS=( | ||
| "@php ${test_installer} --no-interaction --uri=https://github.com/drevops/vortex.git@stable # 0" | ||
| "Using installer script from local path: ${test_installer}" | ||
| ) | ||
|
|
||
| mocks="$(run_steps "setup")" | ||
| run "${ROOT_DIR}/scripts/vortex/update-vortex.sh" | ||
| run_steps "assert" "${mocks[@]}" | ||
|
|
||
| assert_success | ||
|
|
||
| # Clean up. | ||
| rm -f "${test_installer}" | ||
|
|
||
| popd >/dev/null || exit 1 | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding cleanup in a trap for test reliability.
While the test cleans up the temporary installer file, consider using a trap to ensure cleanup happens even if the test fails.
@test "Local installer path is used when VORTEX_INSTALLER_PATH is set and file exists" {
pushd "${LOCAL_REPO_DIR}" >/dev/null || exit 1
create_global_command_wrapper "curl"
create_global_command_wrapper "php"
# Create temporary installer file.
local test_installer="${LOCAL_REPO_DIR}/test-installer.php"
+ trap "rm -f '${test_installer}'" EXIT
echo "<?php echo 'test installer';" > "${test_installer}"
export VORTEX_INSTALLER_PATH="${test_installer}"
declare -a STEPS=(
"@php ${test_installer} --no-interaction --uri=https://github.com/drevops/vortex.git@stable # 0"
"Using installer script from local path: ${test_installer}"
)
mocks="$(run_steps "setup")"
run "${ROOT_DIR}/scripts/vortex/update-vortex.sh"
run_steps "assert" "${mocks[@]}"
assert_success
- # Clean up.
- rm -f "${test_installer}"
-
popd >/dev/null || exit 1
}📝 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.
| @test "Local installer path is used when VORTEX_INSTALLER_PATH is set and file exists" { | |
| pushd "${LOCAL_REPO_DIR}" >/dev/null || exit 1 | |
| create_global_command_wrapper "curl" | |
| create_global_command_wrapper "php" | |
| # Create temporary installer file. | |
| local test_installer="${LOCAL_REPO_DIR}/test-installer.php" | |
| echo "<?php echo 'test installer';" > "${test_installer}" | |
| export VORTEX_INSTALLER_PATH="${test_installer}" | |
| declare -a STEPS=( | |
| "@php ${test_installer} --no-interaction --uri=https://github.com/drevops/vortex.git@stable # 0" | |
| "Using installer script from local path: ${test_installer}" | |
| ) | |
| mocks="$(run_steps "setup")" | |
| run "${ROOT_DIR}/scripts/vortex/update-vortex.sh" | |
| run_steps "assert" "${mocks[@]}" | |
| assert_success | |
| # Clean up. | |
| rm -f "${test_installer}" | |
| popd >/dev/null || exit 1 | |
| } | |
| @test "Local installer path is used when VORTEX_INSTALLER_PATH is set and file exists" { | |
| pushd "${LOCAL_REPO_DIR}" >/dev/null || exit 1 | |
| create_global_command_wrapper "curl" | |
| create_global_command_wrapper "php" | |
| # Create temporary installer file. | |
| local test_installer="${LOCAL_REPO_DIR}/test-installer.php" | |
| trap "rm -f '${test_installer}'" EXIT | |
| echo "<?php echo 'test installer';" > "${test_installer}" | |
| export VORTEX_INSTALLER_PATH="${test_installer}" | |
| declare -a STEPS=( | |
| "@php ${test_installer} --no-interaction --uri=https://github.com/drevops/vortex.git@stable # 0" | |
| "Using installer script from local path: ${test_installer}" | |
| ) | |
| mocks="$(run_steps "setup")" | |
| run "${ROOT_DIR}/scripts/vortex/update-vortex.sh" | |
| run_steps "assert" "${mocks[@]}" | |
| assert_success | |
| popd >/dev/null || exit 1 | |
| } |
🤖 Prompt for AI Agents
In .vortex/tests/bats/unit/update-vortex.bats around lines 60 to 87, the test
creates a temporary installer file and deletes it at the end, but if the test
fails or exits early, the file may not be cleaned up. To fix this, add a trap
command at the start of the test to remove the temporary installer file on EXIT,
ensuring cleanup always occurs regardless of test success or failure.
a12ffe6 to
7a79cfd
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
.ahoy.yml (1)
270-271: Still missing quotes around$@– repeats earlier reviewUnquoted
$@will break when arguments contain whitespace (e.g.--ref "feature branch").
Previous review already requested the fix; please apply it.-cmd: ./scripts/vortex/update-vortex.sh $@ +cmd: ./scripts/vortex/update-vortex.sh "$@".vortex/installer/src/Utils/Config.php (1)
22-24: No fallback → upgrading users with old env-vars will breakRenaming the constants is fine, but existing pipelines that export
VORTEX_INSTALL_REPO/REFwill now resolve tonull.
Implement a backward-compat shim before the first read, e.g.:+// Back-compat: honour legacy env-vars if new ones are unset. +if (!Env::get(self::REPO) && Env::get('VORTEX_INSTALL_REPO')) { + $this->set(self::REPO, Env::get('VORTEX_INSTALL_REPO'), true); +} +if (!Env::get(self::REF) && Env::get('VORTEX_INSTALL_REF')) { + $this->set(self::REF, Env::get('VORTEX_INSTALL_REF'), true); +}.vortex/tests/bats/e2e/update-vortex.bats (1)
52-53: Consider capturing composer output for debugging.While redirecting composer output to
/dev/nullreduces noise, it might make debugging failures more difficult. Consider capturing the output in a variable or log file for potential debugging needs..vortex/installer/tests/Unit/UnitTestCase.php (1)
98-102: Fix line length violations in the comment.The comment explaining the early exit logic exceeds the 80-character limit. Please reformat to comply with coding standards.
- // Early exit after 4 pattern replacements to prevent excessive - // replacements and optimize performance. The threshold of 4 was chosen - // based on typical file modification patterns - most files require 1-3 - // replacements, so 4 provides a reasonable safety margin while preventing - // runaway operations. + // Early exit after 4 pattern replacements to prevent excessive + // replacements and optimize performance. The threshold of 4 was + // chosen based on typical file modification patterns - most files + // require 1-3 replacements, so 4 provides a reasonable safety + // margin while preventing runaway operations..vortex/tests/bats/unit/update-vortex.bats (1)
60-87: Add trap for reliable cleanup of temporary files.While the test cleans up the temporary installer file, using a trap ensures cleanup happens even if the test fails.
@test "Local installer path is used when VORTEX_INSTALLER_PATH is set and file exists" { pushd "${LOCAL_REPO_DIR}" >/dev/null || exit 1 create_global_command_wrapper "curl" create_global_command_wrapper "php" # Create temporary installer file. local test_installer="${LOCAL_REPO_DIR}/test-installer.php" + trap "rm -f '${test_installer}'" EXIT echo "<?php echo 'test installer';" > "${test_installer}" export VORTEX_INSTALLER_PATH="${test_installer}" declare -a STEPS=( "@php ${test_installer} --no-interaction --uri=https://github.com/drevops/vortex.git@stable # 0" "Using installer script from local path: ${test_installer}" ) mocks="$(run_steps "setup")" run "${ROOT_DIR}/scripts/vortex/update-vortex.sh" run_steps "assert" "${mocks[@]}" assert_success - # Clean up. - rm -f "${test_installer}" - popd >/dev/null || exit 1 }.vortex/installer/tests/Unit/DownloaderTest.php (1)
30-238: Comprehensive test coverage with well-named test cases.The valid test cases provide excellent coverage across all URI formats mentioned in the update script documentation. The descriptive naming makes it easy to understand what each test validates.
Note: A previous review suggested organizing this large data provider into smaller grouped methods for better maintainability, which would be a good improvement for the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (22)
.vortex/installer/tests/Fixtures/install/_baseline/.ahoy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/.github/workflows/assign-author.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/.github/workflows/draft-release-notes.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/.github/workflows/update-dependencies.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/.ignorecontentis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/composer.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/package.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/web/themes/custom/star_wars/yarn.lockis excluded by!**/yarn.lock,!**/*.lock,!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/_baseline/yarn.lockis excluded by!**/yarn.lock,!**/*.lock,!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/ciprovider_circleci/.circleci/config.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/deploy_type_all_circleci/.circleci/config.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/deploy_type_none_circleci/.circleci/config.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/deploy_type_none_gha/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/deps_updates_provider_ci_circleci/.circleci/config.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/profile_the_empire/web/profiles/custom/the_empire/the_empire.info.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/provision_profile/.github/workflows/build-test-deploy.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/services_no_clamav/composer.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/services_none/composer.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/install/timezone_circleci/.circleci/config.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/version_replacement/expected/composer.jsonis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/version_replacement/expected/update-dependencies.ymlis excluded by!.vortex/installer/tests/Fixtures/**
📒 Files selected for processing (17)
.ahoy.yml(1 hunks).vortex/docs/content/contributing/maintenance/tests.mdx(2 hunks).vortex/docs/content/workflows/variables.mdx(2 hunks).vortex/installer/src/Utils/Config.php(1 hunks).vortex/installer/src/Utils/Downloader.php(1 hunks).vortex/installer/tests/Unit/DownloaderTest.php(1 hunks).vortex/installer/tests/Unit/UnitTestCase.php(3 hunks).vortex/tests/bats/_helper.bash(1 hunks).vortex/tests/bats/_helper.deployment.bash(1 hunks).vortex/tests/bats/e2e/update-vortex.bats(3 hunks).vortex/tests/bats/unit/deploy-container-registry.bats(2 hunks).vortex/tests/bats/unit/login-container-registry.bats(2 hunks).vortex/tests/bats/unit/update-vortex.bats(1 hunks).vortex/tests/phpunit/Traits/Steps/StepPrepareSutTrait.php(1 hunks).vortex/tests/test.common.sh(1 hunks)scripts/vortex/login-container-registry.sh(1 hunks)scripts/vortex/update-vortex.sh(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
.vortex/tests/bats/unit/deploy-container-registry.bats (1)
.vortex/tests/bats/_helper.deployment.bash (1)
create_docker_config_file(164-168)
.vortex/tests/test.common.sh (3)
.vortex/tests/test.workflow.sh (1)
bats(31-40).vortex/tests/test.deployment.sh (1)
bats(29-38).vortex/tests/test.postbuild.sh (1)
bats(20-29)
.vortex/tests/bats/unit/update-vortex.bats (1)
.vortex/tests/bats/_helper.bash (1)
create_global_command_wrapper(1390-1399)
scripts/vortex/update-vortex.sh (16)
scripts/vortex/login-container-registry.sh (2)
fail(44-44)note(40-40)scripts/vortex/deploy-container-registry.sh (2)
fail(42-42)note(38-38)scripts/vortex/deploy-webhook.sh (2)
fail(30-30)note(26-26)scripts/vortex/deploy-lagoon.sh (2)
fail(83-83)note(79-79)scripts/vortex/download-db-container-registry.sh (2)
fail(35-35)note(31-31)scripts/vortex/download-db-acquia.sh (2)
fail(56-56)note(52-52)scripts/vortex/doctor.sh (2)
fail(72-72)note(68-68)scripts/vortex/download-db-ftp.sh (2)
fail(42-42)note(38-38)scripts/vortex/download-db-url.sh (2)
fail(31-31)note(27-27)scripts/vortex/download-db-lagoon.sh (2)
fail(77-77)note(73-73)scripts/vortex/export-db.sh (2)
fail(28-28)note(24-24)scripts/vortex/export-db-image.sh (2)
fail(36-36)note(32-32)scripts/vortex/notify-newrelic.sh (2)
fail(53-53)note(49-49)scripts/vortex/notify-webhook.sh (2)
fail(45-45)note(41-41)scripts/vortex/notify-jira.sh (2)
fail(54-54)note(50-50)scripts/vortex/export-db-file.sh (2)
fail(24-24)note(20-20)
.vortex/installer/tests/Unit/UnitTestCase.php (1)
.vortex/installer/src/Utils/File.php (2)
File(10-120)replaceContentAsync(59-83)
.vortex/tests/bats/e2e/update-vortex.bats (1)
.vortex/tests/bats/_helper.bash (8)
substep(1327-1329)git_init(1283-1293)git_add_all_commit(1271-1281)run_installer_quiet(1084-1123)assert_files_present(182-216)install_dependencies_stub(1130-1156)git_add(1248-1252)git_commit(1260-1269)
⏰ 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). (13)
- GitHub Check: build (0)
- GitHub Check: build (1)
- GitHub Check: vortex-test-workflow (2)
- GitHub Check: vortex-test-workflow (1)
- GitHub Check: vortex-test-workflow (3)
- GitHub Check: vortex-test-workflow (0)
- GitHub Check: vortex-test-deployment (0)
- GitHub Check: vortex-test-common
- GitHub Check: vortex-test-deployment (1)
- GitHub Check: vortex-test-installer (8.4)
- GitHub Check: vortex-test-installer (8.3)
- GitHub Check: vortex-test-installer (8.2)
- GitHub Check: vortex-test-docs
🔇 Additional comments (21)
.vortex/installer/src/Utils/Downloader.php (1)
48-48: LGTM! Error message correction improves clarity.The exception message now accurately reflects that the issue is with a local repository URI format (file://) rather than a remote one.
.vortex/tests/phpunit/Traits/Steps/StepPrepareSutTrait.php (1)
53-53: Approved: Environment variable rename is consistent
A global search forVORTEX_INSTALL_REPOreturned no remaining references. The change toVORTEX_INSTALL_TEMPLATE_REPOis clear, descriptive, and aligns with the standardization goals. LGTM!.vortex/tests/bats/unit/deploy-container-registry.bats (1)
36-36: LGTM! Function renaming aligns with improved Docker config simulation.The change from
provision_docker_config_filetocreate_docker_config_fileis consistent with the updated helper function that now creates a proper JSON Docker authentication config, providing more realistic test conditions.Also applies to: 90-90
.vortex/docs/content/contributing/maintenance/tests.mdx (1)
125-125: LGTM! Documentation updated to reflect environment variable renaming.The documentation examples now use the correct environment variable name
VORTEX_INSTALL_TEMPLATE_REPO, maintaining consistency with the codebase changes.Also applies to: 163-163
scripts/vortex/login-container-registry.sh (1)
55-55: LGTM! Bug fix corrects Docker config file detection.The change properly checks for the existence of
${DOCKER_CONFIG}/config.jsoninstead of treatingDOCKER_CONFIGas a file. This correctly handles the Docker configuration directory structure whereconfig.jsonis the actual configuration file within the directory..vortex/tests/bats/_helper.bash (1)
1086-1089: LGTM – env-var rename reflected correctlyThe helper now exports
VORTEX_INSTALL_TEMPLATE_REPO, matching the new constant.
No further action required..vortex/tests/bats/e2e/update-vortex.bats (1)
57-60: Good test coverage improvements!The addition of:
- Clear documentation for the installer URL override mechanism
- Assertion to verify installer script cleanup after update
These changes enhance test reliability and maintainability.
Also applies to: 77-78
.vortex/tests/bats/unit/login-container-registry.bats (2)
28-43: Improved test clarity and structure.The changes enhance the test by:
- Using a more descriptive test name
- Properly setting up DOCKER_CONFIG with BUILD_DIR
- Ensuring consistent function naming with
create_docker_config_file
73-81: Good refactoring to use the STEPS pattern.The refactoring improves test maintainability by using the declarative STEPS array pattern, making the expected command sequence and outputs more explicit and easier to understand.
.vortex/docs/content/workflows/variables.mdx (1)
1142-1148: Comprehensive documentation updates for new installer variables.The documentation accurately reflects the new functionality:
VORTEX_INSTALLER_PATHfor local installer script overrideVORTEX_INSTALLER_URL_CACHE_BUSTfor preventing stale downloads- Renamed
VORTEX_INSTALL_TEMPLATE_REPOwith detailed URI format examplesThe examples are particularly helpful for users understanding the various supported formats.
Also applies to: 1158-1164, 1166-1173
.vortex/installer/tests/Unit/UnitTestCase.php (1)
48-61: Excellent refactoring for performance and modularity.The refactoring brings several improvements:
- Async processing pattern enables better performance for multiple files
- File extension filtering prevents unnecessary processing
- Extracted method improves testability and separation of concerns
.vortex/tests/bats/unit/update-vortex.bats (1)
11-205: Excellent comprehensive test coverage!This test suite provides thorough coverage of the update-vortex.sh functionality:
- Environment variable handling and defaults
- Local installer path support with proper error handling
- Various URI formats (HTTP, file://, local paths, git SSH)
- Graceful failure scenarios
The consistent use of the STEPS array pattern makes the tests highly maintainable.
scripts/vortex/update-vortex.sh (7)
3-3: Improved clarity in script description.The updated description better clarifies that this script updates Vortex from the template repository, which aligns with the standardized naming conventions.
14-29: Comprehensive documentation with excellent URI format examples.The documentation provides clear examples covering all supported URI formats (HTTPS, SSH, file URLs, local paths) with various reference types. This will help users understand the flexibility of the script.
29-29: Well-structured environment variable with backward compatibility.The renamed variable
VORTEX_INSTALL_TEMPLATE_REPOfollows the standardized naming convention and maintains support for command-line argument override, providing good flexibility.
34-40: Good additions for cache control and local installer support.The cache busting parameter using timestamp is effective for avoiding cached downloads, and the local installer path override provides valuable flexibility for development and testing scenarios.
51-55: Robust pre-flight checks with proper error handling.The command validation uses portable
command -vchecking and provides clear error messages. The loop structure efficiently validates all required dependencies.
57-71: Well-implemented conditional installer sourcing with proper validation.The logic cleanly handles both local and remote installer scenarios with appropriate validation:
- Local path existence verification prevents runtime errors
- Download failure handling with descriptive error messages
- Cache busting correctly applied to prevent stale downloads
73-73: Clean final execution with appropriate parameters.The PHP execution correctly uses the validated installer path and passes the necessary parameters (
--no-interactionfor automation,--uriwith the template repository)..vortex/installer/tests/Unit/DownloaderTest.php (2)
1-28: Excellent test structure following modern PHP and PHPUnit practices.The class demonstrates:
- Modern PHP 8+ syntax with strict types
- Proper PHPUnit attributes usage
- Well-structured test method with comprehensive parameter handling
- Appropriate exception testing and assertions
239-427: Thorough error case coverage with proper exception validation.The invalid test cases comprehensively cover various failure scenarios:
- Malformed URLs and SSH formats
- Invalid reference formats and commit hash lengths
- Edge cases with special characters and double @ symbols
The exception message validation ensures users receive meaningful error feedback.
| create_docker_config_file() { | ||
| fixture_prepare_dir "${BUILD_DIR}/.docker" | ||
| touch "${BUILD_DIR}/.docker/config.json" | ||
| echo "{$1:-docker.io}" > "${BUILD_DIR}/.docker/config.json" | ||
| echo "{\"auths\": {\"${1:-docker.io}\": {\"auth\": \"bXl1c2VybmFtZTpteXBhc3N3b123\"}}}" >"${BUILD_DIR}/.docker/config.json" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Hard-coded auth blob limits reuse
create_docker_config_file writes a fixed base64 string that decodes to myusername:mypassw123.
This makes the helper unusable for alternative creds and leaks a dummy secret into git history.
Refactor to accept user / pass (or pre-encoded auth) as arguments; fall back to the current dummy to keep existing tests green.
-echo "{\"auths\": {\"${1:-docker.io}\": {\"auth\": \"bXl1c2VybmFtZTpteXBhc3N3b123\"}}}" >"${BUILD_DIR}/.docker/config.json"
+local registry="${1:-docker.io}"
+local auth_b64="${2:-$(printf "%s:%s" "${TEST_DOCKER_USER:-user}" "${TEST_DOCKER_PASS:-pass}" | base64 -w0)}"
+echo "{\"auths\": {\"${registry}\": {\"auth\": \"${auth_b64}\"}}}" >"${BUILD_DIR}/.docker/config.json"🤖 Prompt for AI Agents
In .vortex/tests/bats/_helper.deployment.bash around lines 164 to 168, the
create_docker_config_file function uses a hard-coded base64 auth string,
limiting reuse and exposing a dummy secret. Modify the function to accept
username and password parameters (or a pre-encoded auth string) and generate the
auth field dynamically from these inputs. If no arguments are provided, fall
back to the existing dummy credentials to maintain compatibility with current
tests.
| bats "${TEST_DIR}/bats/unit/login-container-registry.bats" | ||
| bats "${TEST_DIR}/bats/unit/notify.bats" | ||
| bats "${TEST_DIR}/bats/unit/provision.bats" | ||
| bats "${TEST_DIR}/bats/unit/setup-ssh.bats" | ||
| bats "${TEST_DIR}/bats/unit/update-vortex.bats" |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
List is drifting out-of-sync → extract to a single array
The growing hard-coded list of bats invocations is already duplicated across other test..sh launchers and is easy to forget when new unit files are added.
Wrap the filenames into an array and iterate once – this keeps ordering explicit yet avoids copy-paste maintenance.
-# Unit tests
-bats "${TEST_DIR}/bats/unit/login-container-registry.bats"
-bats "${TEST_DIR}/bats/unit/provision.bats"
-bats "${TEST_DIR}/bats/unit/setup-ssh.bats"
-bats "${TEST_DIR}/bats/unit/update-vortex.bats"
+UNIT_SPECS=(
+ login-container-registry
+ provision
+ setup-ssh
+ update-vortex
+)
+for spec in "${UNIT_SPECS[@]}"; do
+ bats "${TEST_DIR}/bats/unit/${spec}.bats"
+doneCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In .vortex/tests/test.common.sh around lines 44 to 48, the multiple hard-coded
bats test file invocations should be replaced by defining an array containing
all the test filenames and then iterating over this array to call bats on each
file. This change centralizes the list, making it easier to maintain and update
without duplicating the list in multiple places.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Refactor