(feat): auto-detect cli params to force non-interactive installer#11636
(feat): auto-detect cli params to force non-interactive installer#11636
Conversation
📝 WalkthroughWalkthroughThis change modifies the installation task initialization logic in a single file. A new private method Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR adds auto-detection of explicit CLI parameters to force the installer into non-interactive (CLI) mode, bypassing the web installer when flags like
Confidence Score: 2/5
Important Files Changed
Reviews (1): Last reviewed commit: "(feat): auto-detect cli params to force ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| { | ||
| $argv = $_SERVER['argv'] ?? []; | ||
| foreach ($argv as $arg) { | ||
| if (\str_starts_with($arg, '--') && !\str_starts_with($arg, '--interactive')) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; |
There was a problem hiding this comment.
-- detection causes false positives
The function returns true for any argument beginning with -- (other than --interactive). This means flags like --help or --version — which a user might pass just to inspect the command — will silently bypass the web installer and drop the user into CLI mode. It will also capture framework-level double-dash options that have nothing to do with installer configuration.
A safer approach is to only match the known installer params (--http-port, --https-port, --organization, --image, --no-start, --database) rather than using an open-ended prefix check:
private function hasExplicitCliParams(): bool
{
$installerParams = [
'--http-port', '--https-port', '--organization',
'--image', '--no-start', '--database',
];
$argv = $_SERVER['argv'] ?? [];
foreach ($argv as $arg) {
$argName = explode('=', $arg, 2)[0];
if (in_array($argName, $installerParams, true)) {
return true;
}
}
return false;
}This is both explicit about intent and immune to false positives from unrelated flags.
| 'domain' => $domain, | ||
| 'database' => $database, | ||
| 'hostIp' => $hostIp !== $domain ? $hostIp : null, | ||
| 'ip' => $hostIp !== $domain ? $hostIp : null, |
There was a problem hiding this comment.
hostIp → ip rename may silently break the Growth API contract
The analytics payload key was renamed from hostIp to ip, but this is a server-side contract with https://growth.appwrite.io/v1/analytics. If the receiving API still expects the field named hostIp, the IP value will silently be discarded on the server side with no error returned to the caller (the catch (\Throwable) block at line 784 swallows any response failures too).
Please confirm that the Growth API has already been (or will atomically be) updated to accept ip before this change is deployed.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Appwrite/Platform/Tasks/Install.php (1)
1270-1276: Consider using an exact match for--interactiveexclusion.The current check
!\str_starts_with($arg, '--interactive')also excludes any argument starting with--interactive(e.g., a hypothetical--interactive-mode). For more precise matching, consider checking the exact flag or its value variants.♻️ Suggested refinement
private function hasExplicitCliParams(): bool { $argv = $_SERVER['argv'] ?? []; foreach ($argv as $arg) { - if (\str_starts_with($arg, '--') && !\str_starts_with($arg, '--interactive')) { + if (\str_starts_with($arg, '--') && $arg !== '--interactive' && !\str_starts_with($arg, '--interactive=')) { return true; } } return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Appwrite/Platform/Tasks/Install.php` around lines 1270 - 1276, The hasExplicitCliParams method currently treats any argument beginning with --interactive as the same as the exact flag, which incorrectly excludes flags like --interactive-mode; update the check so it only ignores the exact flag and legitimate value forms (e.g., '--interactive' or '--interactive=...') by replacing the negative str_starts_with condition with a precise test: return true for any --... argument except when $arg === '--interactive' or when it begins with '--interactive='; reference hasExplicitCliParams and the $argv loop to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Appwrite/Platform/Tasks/Install.php`:
- Around line 1270-1276: The hasExplicitCliParams method currently treats any
argument beginning with --interactive as the same as the exact flag, which
incorrectly excludes flags like --interactive-mode; update the check so it only
ignores the exact flag and legitimate value forms (e.g., '--interactive' or
'--interactive=...') by replacing the negative str_starts_with condition with a
precise test: return true for any --... argument except when $arg ===
'--interactive' or when it begins with '--interactive='; reference
hasExplicitCliParams and the $argv loop to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bbca9f83-419e-4e1d-82f2-7ba512b6ae31
📒 Files selected for processing (1)
src/Appwrite/Platform/Tasks/Install.php
✨ Benchmark results
⚡ Benchmark Comparison
|
|
Found 1 test error on Blacksmith runners: Error
|
1 participant
What does this PR do?
(Provide a description of what this PR does and why it's needed.)
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Screenshots may also be helpful.)
Related PRs and Issues
Checklist