[#1663] Added removal of installer after installation success. #1826
Conversation
WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant InstallCommand
participant Config
User->>CLI: Run installer (with/without --no-cleanup)
CLI->>InstallCommand: Parse options
InstallCommand->>Config: Store no-cleanup flag
InstallCommand->>InstallCommand: Execute installation
InstallCommand->>InstallCommand: Register shutdown cleanup
Note over InstallCommand: On shutdown after success
InstallCommand->>InstallCommand: Call cleanup()
InstallCommand->>Config: Check no-cleanup flag
alt no-cleanup is set
InstallCommand-->>InstallCommand: Skip PHAR deletion
else
InstallCommand->>InstallCommand: Delete PHAR file
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Suggested labels
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/Command/InstallCommand.phpERROR: Referenced sniff "Drupal" does not exist Run "phpcs --help" for usage information .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
✨ 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 (
|
![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 selected for processing (4)
.vortex/installer/src/Command/InstallCommand.php(5 hunks).vortex/installer/src/Utils/Config.php(1 hunks).vortex/installer/tests/Functional/PharTest.php(1 hunks).vortex/tests/phpunit/Functional/FunctionalTestCase.php(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
.vortex/tests/phpunit/Functional/FunctionalTestCase.php (1)
Learnt from: AlexSkrypnyk
PR: #1693
File: .vortex/installer/tests/Fixtures/install/deploy_type_none_gha/.github/workflows/build-test-deploy.yml:1-4
Timestamp: 2025-06-01T08:08:59.071Z
Learning: Test fixture files (typically located in directories containing "fixture" or "test") should not be reviewed for code quality issues as they may intentionally contain incomplete, invalid, or malformed data for testing purposes.
.vortex/installer/tests/Functional/PharTest.php (2)
Learnt from: AlexSkrypnyk
PR: #1693
File: .vortex/installer/src/Prompts/Handlers/Internal.php:52-52
Timestamp: 2025-06-01T08:10:15.903Z
Learning: The File utility class in .vortex/installer/src/Utils/File.php extends AlexSkrypnyk\File\File, and methods like collapseRepeatedEmptyLines() are available from this external parent class dependency.
Learnt from: AlexSkrypnyk
PR: drevops/vortex#0
File: :0-0
Timestamp: 2025-05-29T12:15:32.188Z
Learning: Do not review files in .vortex/installer/tests/Fixtures/install directory as they are test fixtures.
⏰ 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 (1)
- GitHub Check: vortex-test-workflow (0)
- GitHub Check: vortex-test-workflow (3)
- GitHub Check: vortex-test-workflow (2)
- GitHub Check: vortex-test-deployment (0)
- GitHub Check: vortex-test-deployment (1)
- GitHub Check: vortex-test-common
- GitHub Check: vortex-test-installer (8.4)
- GitHub Check: vortex-test-installer (8.2)
- GitHub Check: vortex-test-installer (8.3)
- GitHub Check: vortex-test-docs
🔇 Additional comments (12)
.vortex/tests/phpunit/Functional/FunctionalTestCase.php (1)
44-44: LGTM! Grammar correction improves readability.The comment correction from "do setting up" to "so setting up" improves the clarity of the explanation for why the 'Star Wars' theme is used for tests.
.vortex/installer/src/Utils/Config.php (1)
40-41: LGTM! Constant follows established naming pattern.The new
NO_CLEANUPconstant is properly named and follows the consistent pattern used by other configuration constants in this class..vortex/installer/src/Command/InstallCommand.php (5)
40-40: LGTM! Constant follows established naming pattern.The new
OPTION_NO_CLEANUPconstant is properly named and consistent with other option constants in this class.
83-83: LGTM! Option registration is correct.The
--no-cleanupoption is properly registered withInputOption::VALUE_NONE, which is appropriate for a boolean flag.
169-171: LGTM! Appropriate use of shutdown function for cleanup.Registering the cleanup as a shutdown function ensures it only runs after successful installation, which aligns with the comment's intention to allow re-running the installer if it fails.
252-254: LGTM! Config setting correctly handles the option.The no-cleanup flag is properly read from the options array and cast to boolean before storing in the configuration.
456-469: LGTM! Cleanup implementation is robust.The cleanup method properly:
- Checks the no-cleanup flag before proceeding
- Uses
\Phar::running(FALSE)to get the current PHAR path- Validates file existence before attempting deletion
- Uses error suppression on
unlink()to handle potential permission issues gracefully.vortex/installer/tests/Functional/PharTest.php (5)
27-38: LGTM! Test setup is well-structured.The setUp method properly initializes the test environment, sets up the 'star_wars' directory for consistent test data, and builds the required PHAR file.
46-53: LGTM! Test covers default cleanup behavior.This test properly verifies that the PHAR file is removed after successful installation when no flags are specified.
55-62: LGTM! Test covers no-cleanup flag behavior.This test correctly verifies that the PHAR file remains when the
--no-cleanupflag is used.
74-88: LGTM! Helper method is well-implemented.The
buildPharmethod efficiently handles PHAR building with proper caching (only builds if needed) and includes appropriate assertions to verify the build process.
90-115: LGTM! Comprehensive test helper with good documentation.The
runInstallationWithPharmethod properly handles option parsing and includes good documentation about the limitations with interactive mode testing.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1826 +/- ##
===========================================
- Coverage 74.52% 74.14% -0.38%
===========================================
Files 84 84
Lines 4761 4835 +74
Branches 35 35
===========================================
+ Hits 3548 3585 +37
- Misses 1213 1250 +37 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
AlexSkrypnyk
changed the title
AlexSkrypnyk
force-pushed
the
feature/1663-installer-remove-itself
branch
from
28f130f to
3f7601f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
.github/workflows/vortex-test-installer.yml (1)
18-21: PHP 8.4 image is not yet available inshivammathur/setup-php– build will fail
setup-php@v2currently supports released versions and thenightlychannel. Using the literal8.4string will cause the workflow to error out once it reaches that matrix entry.-php-versions: ['8.2', '8.3', '8.4'] +php-versions: ['8.2', '8.3', 'nightly'] # or drop until 8.4 is releasedAlternatively, gate the matrix value behind a feature flag so CI remains green.
♻️ Duplicate comments (1)
.github/workflows/vortex-test-installer.yml (1)
74-74: Same concern as above – duplicate shadowing ofGITHUB_TOKENReplicate the fix from the “Run tests” step so both places use the dedicated env var.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/vortex-test-installer.yml(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
.github/workflows/vortex-test-installer.yml (2)
Learnt from: AlexSkrypnyk
PR: #1816
File: .docker/cli.dockerfile:72-76
Timestamp: 2025-07-23T01:16:30.963Z
Learning: In the Vortex project, unauthenticated Composer installs should be allowed, so GitHub token secrets should not be marked as required=true in Docker build mounts. The conditional check if [ -s /run/secrets/github_token ] is the preferred approach to allow builds to proceed without a token when needed.
Learnt from: AlexSkrypnyk
PR: drevops/vortex#0
File: :0-0
Timestamp: 2025-05-29T12:15:32.188Z
Learning: Do not review files in .vortex/installer/tests/Fixtures/install directory as they are test fixtures.
⏰ 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 (3)
- GitHub Check: vortex-test-deployment (0)
- GitHub Check: vortex-test-deployment (1)
- GitHub Check: vortex-test-workflow (0)
- GitHub Check: vortex-test-workflow (1)
- GitHub Check: vortex-test-workflow (2)
- GitHub Check: vortex-test-common
- GitHub Check: vortex-test-installer (8.2)
- GitHub Check: vortex-test-installer (8.3)
- GitHub Check: vortex-test-installer (8.4)
- GitHub Check: vortex-test-docs
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
.vortex/installer/tests/Functional/PharTest.php (1)
82-82: Fix misleading comment in test.The comment mentions "--no-cleanup flag" but this test actually uses the
--helpflag. The assertion is correct, but the comment should reflect the actual behavior being tested.- $this->assertFileExists($this->pharFile, 'PHAR file should NOT be removed when --no-cleanup flag is used'); + $this->assertFileExists($this->pharFile, 'PHAR file should NOT be removed when --help flag is used');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.vortex/installer/src/Utils/Downloader.php(3 hunks).vortex/installer/tests/Functional/PharTest.php(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
.vortex/installer/tests/Functional/PharTest.php (3)
Learnt from: AlexSkrypnyk
PR: #1693
File: .vortex/installer/src/Prompts/Handlers/Internal.php:52-52
Timestamp: 2025-06-01T08:10:15.903Z
Learning: The File utility class in .vortex/installer/src/Utils/File.php extends AlexSkrypnyk\File\File, and methods like collapseRepeatedEmptyLines() are available from this external parent class dependency.
Learnt from: AlexSkrypnyk
PR: drevops/vortex#0
File: :0-0
Timestamp: 2025-05-29T12:15:32.188Z
Learning: Do not review files in .vortex/installer/tests/Fixtures/install directory as they are test fixtures.
Learnt from: AlexSkrypnyk
PR: #1697
File: .vortex/installer/tests/Fixtures/install/provision_database_lagoon/docs/onboarding.md:16-17
Timestamp: 2025-06-02T00:41:36.321Z
Learning: DO NOT EVER REVIEW FILES IN .vortex/installer/tests/Fixtures/ directory - these are test fixtures and should not be reviewed under any circumstances.
.vortex/installer/src/Utils/Downloader.php (1)
Learnt from: AlexSkrypnyk
PR: #1693
File: .vortex/installer/src/Prompts/Handlers/Internal.php:52-52
Timestamp: 2025-06-01T08:10:15.903Z
Learning: The File utility class in .vortex/installer/src/Utils/File.php extends AlexSkrypnyk\File\File, and methods like collapseRepeatedEmptyLines() are available from this external parent class dependency.
⏰ 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 (3)
- GitHub Check: vortex-test-workflow (0)
- GitHub Check: vortex-test-workflow (1)
- GitHub Check: vortex-test-deployment (1)
- GitHub Check: vortex-test-deployment (0)
- 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 (8)
.vortex/installer/tests/Functional/PharTest.php (5)
1-26: Well-structured test class setup.The class properly extends FunctionalTestCase and uses appropriate traits for process management and terminal UI interaction. Good use of PHPUnit attributes for code coverage.
30-49: Proper test setup with clear documentation.Good practice to build the PHAR once in
setUpBeforeClassand copy it for each test. The comment explaining the "star_wars" directory choice is helpful.
51-64: Comprehensive test for default PHAR cleanup behavior.The test properly verifies all aspects: successful execution, correct output, file creation, and PHAR cleanup.
66-73: Correct test for --no-cleanup flag behavior.The test properly verifies that the PHAR file is retained when the --no-cleanup flag is used.
85-129: Well-implemented helper methods with good documentation.The
buildPharmethod has proper error handling, andrunInstallationWithPharcorrectly handles command construction with clear documentation about the non-interactive mode requirement..vortex/installer/src/Utils/Downloader.php (3)
91-94: Excellent refactoring of the download process.Replacing the inline pipeline with discrete steps improves error handling, testability, and maintainability. The temporary file is properly cleaned up after use.
114-117: Consistent refactoring for local downloads.Good to see the same approach applied to local downloads, maintaining consistency across the codebase.
218-267: Robust archive validation with proper format detection.Excellent implementation checking magic bytes for both gzip and tar formats. Good error handling and resource management with proper file handle closure.
AlexSkrypnyk
force-pushed
the
feature/1663-installer-remove-itself
branch
from
0d380e2 to
ca1553a
Compare
AlexSkrypnyk
force-pushed
the
feature/1663-installer-remove-itself
branch
from
ca1553a to
fc55d3d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.github/workflows/vortex-test-installer.yml (2)
47-48: Avoid shadowing the built-in GITHUB_TOKEN; export your PAT under PACKAGE_TOKENThis maintains the previously identified issue of overwriting the default
GITHUB_TOKEN. As recommended in the past review, consider exposing the secret under its own name:env: - GITHUB_TOKEN: ${{ secrets.PACKAGE_TOKEN }} + PACKAGE_TOKEN: ${{ secrets.PACKAGE_TOKEN }}Then update your Composer authentication to consume
PACKAGE_TOKENdirectly.
73-74: Same shadowing issue in Test PHAR stepThis step also shadows the built-in
GITHUB_TOKEN. Apply the same fix as the previous step:env: - GITHUB_TOKEN: ${{ secrets.PACKAGE_TOKEN }} + PACKAGE_TOKEN: ${{ secrets.PACKAGE_TOKEN }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/vortex-test-installer.yml(2 hunks).vortex/installer/src/Command/InstallCommand.php(6 hunks).vortex/installer/src/Utils/Config.php(1 hunks).vortex/installer/src/Utils/Downloader.php(3 hunks).vortex/installer/tests/Functional/PharTest.php(1 hunks).vortex/tests/phpunit/Functional/FunctionalTestCase.php(1 hunks)scripts/vortex/update-vortex.sh(0 hunks)
💤 Files with no reviewable changes (1)
- scripts/vortex/update-vortex.sh
🧰 Additional context used
🧠 Learnings (4)
.vortex/tests/phpunit/Functional/FunctionalTestCase.php (3)
Learnt from: AlexSkrypnyk
PR: #1693
File: .vortex/installer/tests/Fixtures/install/deploy_type_none_gha/.github/workflows/build-test-deploy.yml:1-4
Timestamp: 2025-06-01T08:08:59.071Z
Learning: Test fixture files (typically located in directories containing "fixture" or "test") should not be reviewed for code quality issues as they may intentionally contain incomplete, invalid, or malformed data for testing purposes.
Learnt from: AlexSkrypnyk
PR: #1697
File: .vortex/installer/tests/Fixtures/install/provision_database_lagoon/docs/onboarding.md:16-17
Timestamp: 2025-06-02T00:41:36.321Z
Learning: DO NOT EVER REVIEW FILES IN .vortex/installer/tests/Fixtures/ directory - these are test fixtures and should not be reviewed under any circumstances.
Learnt from: AlexSkrypnyk
PR: drevops/vortex#0
File: :0-0
Timestamp: 2025-05-29T12:15:32.188Z
Learning: Do not review files in .vortex/installer/tests/Fixtures/install directory as they are test fixtures.
.github/workflows/vortex-test-installer.yml (2)
Learnt from: AlexSkrypnyk
PR: #1816
File: .docker/cli.dockerfile:72-76
Timestamp: 2025-07-23T01:16:30.963Z
Learning: In the Vortex project, unauthenticated Composer installs should be allowed, so GitHub token secrets should not be marked as required=true in Docker build mounts. The conditional check if [ -s /run/secrets/github_token ] is the preferred approach to allow builds to proceed without a token when needed.
Learnt from: AlexSkrypnyk
PR: drevops/vortex#0
File: :0-0
Timestamp: 2025-05-29T12:15:32.188Z
Learning: Do not review files in .vortex/installer/tests/Fixtures/install directory as they are test fixtures.
.vortex/installer/src/Utils/Downloader.php (1)
Learnt from: AlexSkrypnyk
PR: #1693
File: .vortex/installer/src/Prompts/Handlers/Internal.php:52-52
Timestamp: 2025-06-01T08:10:15.903Z
Learning: The File utility class in .vortex/installer/src/Utils/File.php extends AlexSkrypnyk\File\File, and methods like collapseRepeatedEmptyLines() are available from this external parent class dependency.
.vortex/installer/tests/Functional/PharTest.php (3)
Learnt from: AlexSkrypnyk
PR: #1693
File: .vortex/installer/src/Prompts/Handlers/Internal.php:52-52
Timestamp: 2025-06-01T08:10:15.903Z
Learning: The File utility class in .vortex/installer/src/Utils/File.php extends AlexSkrypnyk\File\File, and methods like collapseRepeatedEmptyLines() are available from this external parent class dependency.
Learnt from: AlexSkrypnyk
PR: drevops/vortex#0
File: :0-0
Timestamp: 2025-05-29T12:15:32.188Z
Learning: Do not review files in .vortex/installer/tests/Fixtures/install directory as they are test fixtures.
Learnt from: AlexSkrypnyk
PR: #1697
File: .vortex/installer/tests/Fixtures/install/provision_database_lagoon/docs/onboarding.md:16-17
Timestamp: 2025-06-02T00:41:36.321Z
Learning: DO NOT EVER REVIEW FILES IN .vortex/installer/tests/Fixtures/ directory - these are test fixtures and should not be reviewed under any circumstances.
⏰ 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). (10)
- GitHub Check: build (1)
- GitHub Check: build (0)
- GitHub Check: vortex-test-workflow (2)
- GitHub Check: vortex-test-deployment (1)
- GitHub Check: vortex-test-deployment (0)
- GitHub Check: vortex-test-workflow (3)
- GitHub Check: vortex-test-common
- GitHub Check: vortex-test-workflow (0)
- GitHub Check: vortex-test-workflow (1)
- GitHub Check: vortex-test-installer (8.3)
🔇 Additional comments (19)
.vortex/tests/phpunit/Functional/FunctionalTestCase.php (1)
44-44: LGTM: Improved comment clarityThe grammatical correction from "do" to "so" improves the comment's readability and better explains the reason for the SUT directory setup.
.vortex/installer/src/Utils/Config.php (1)
40-41: LGTM: Consistent constant additionThe new
NO_CLEANUPconstant follows the established naming pattern and integrates well with the existing configuration constants..vortex/installer/src/Command/InstallCommand.php (5)
40-40: LGTM: Consistent option constantThe new
OPTION_NO_CLEANUPconstant follows the established naming pattern for command options.
83-83: LGTM: Well-documented optionThe option is properly configured with clear description and follows the existing pattern of using
InputOption::VALUE_NONEfor boolean flags.
169-171: LGTM: Appropriate use of shutdown functionRegistering the cleanup as a shutdown function ensures it only runs after successful installation, which is the correct behavior for this feature.
253-254: LGTM: Proper config integrationThe no-cleanup flag is correctly stored in the configuration using the established constant and boolean casting.
456-469: LGTM: Safe cleanup implementationThe cleanup method is well-implemented with proper safety checks:
- Respects the no-cleanup flag
- Uses
\Phar::running(FALSE)to safely get the PHAR path- Checks file existence before attempting to unlink
- Uses error suppression appropriately for cleanup operations
.vortex/installer/tests/Functional/PharTest.php (7)
16-17: LGTM: Proper test class structureThe
#[CoversClass(InstallCommand::class)]attribute properly indicates what class this test covers, which is good for coverage reporting.
30-34: LGTM: Appropriate static setupThe
setUpBeforeClassmethod properly ensures the PHAR is built before any tests run, which is efficient for the test suite.
57-64: LGTM: Comprehensive default behavior testThe test properly verifies that:
- Installation succeeds
- Expected output is present
- Required files are created
- PHAR file is cleaned up by default
66-73: LGTM: Proper no-cleanup option testThe test correctly verifies that when
--no-cleanupis used, the PHAR file remains after installation while still creating the expected output files.
75-83: LGTM: Good edge case coverageThe help option test properly verifies that:
- Help is displayed instead of running installation
- No installation artifacts are created
- PHAR file is not removed (since no installation occurred)
85-103: LGTM: Robust PHAR build helperThe
buildPharmethod includes proper error handling and dependency management, ensuring tests can run reliably even when starting from a clean state.
105-130: LGTM: Well-structured test runnerThe
runInstallationWithPharmethod properly:
- Handles option formatting
- Enforces non-interactive mode for reliable testing
- Sets appropriate timeouts for PHAR operations
- Uses proper environment variables
.vortex/installer/src/Utils/Downloader.php (5)
91-94: Good refactoring to improve robustness and error handling.The separation of download, validation, and extraction steps allows for better error handling and debugging compared to the previous shell pipeline approach. The explicit cleanup with
unlink()ensures temporary files don't accumulate.
114-117: Consistent implementation with remote downloads.Good to see the same pattern applied for local downloads, maintaining consistency in the codebase.
182-216: Well-implemented download method with proper security measures.The implementation correctly addresses the previous command injection concerns by using
escapeshellarg()for all dynamic values. Good error handling with cleanup of temporary files on failure.
227-267: Comprehensive archive validation implementation.Good defensive programming with proper validation of file existence, size, and format. The dual format support (gzip and tar) with magic number checking is well-implemented.
333-353: Secure implementation of local archive creation.The git archive command is properly constructed with escaped arguments, addressing the previous security concerns. Good error handling with cleanup on failure.
Closes #1663
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
Chores
Refactor