Repository navigation
feat(operation:run): add --parameter option for runtime operations - #22
Conversation
1e5e867 to
fa14cd1
Compare
fa14cd1 to
7c0af35
Compare
pjcdawkins
left a comment
There was a problem hiding this comment.
Thanks for this, and apologies it sat unreviewed for so long — that is on us, not you.
Starting with the important part: both red checks are ours, not yours. Nothing in your PHP needs to change to make CI pass.
The two CI failures
legacy-php (PHPStan) — the annotation is RunCommand.php:130 EnvironmentDeployment::execRuntimeOperation() invoked with 3 parameters, 2 required. When this ran on 2026-03-02, legacy/composer.lock still pinned platformsh/client 3.0.0-beta2, whose signature was execRuntimeOperation(string $name, string $service). The third array $parameters = [] argument arrived in 3.0.0-beta5, which we pulled in via 8707228 (#32) — merged 2026-03-20, eighteen days after your CI run. Main now pins beta5. Running PHPStan level 8 on your file against the currently vendored client leaves only the pre-existing baselined $service nullable error, so the step passes on a rebase.
check (Security workflow) — the annotation is Dependency review is not supported on this repository. That was a repository setting on our side. It has since been enabled, and check now passes on later PRs including ones from forks. A rebase push re-runs it green.
So: merge or rebase onto current main and both go green. Your only file is otherwise byte-identical to main's, so it should apply cleanly. Since the branch lives in upsun/cli rather than a fork, we can also refresh it for you — say the word if you would rather not bother.
The one change worth making
--parameter currently goes through ArrayArgument::getOption() (RunCommand.php:43 and :129), which splits each value on commas and whitespace (ArrayArgument::split(), legacy/src/Console/ArrayArgument.php:59). That means:
operation:run migrate --parameter "--target=/var/www/my app"
is sent as two parameters, ["--target=/var/www/my", "app"], and no amount of quoting can prevent it — the split happens after the shell and after Symfony's parsing.
This is a functional problem rather than a style preference, because the API treats each parameter as one shell-quoted positional argument (it uses shlex.join), and its own functional tests cover parameters containing whitespace. So a value with a space is a supported case that this option cannot currently express.
Every other SPLIT_HELP user in the CLI passes lists of identifiers or enum values — project IDs, activity states, environment types — none of which contain spaces. The closest analog for opaque user-supplied values is source-operation:run --variable (legacy/src/Command/SourceOperation/RunCommand.php:34), which deliberately uses VALUE_REQUIRED|VALUE_IS_ARRAY with a raw $input->getOption() and no splitting.
Suggested change: drop the ArrayArgument usage and the . ArrayArgument::SPLIT_HELP from the description, read the option directly, and make the help text say that one --parameter equals exactly one parameter, repeatable and order-preserving.
Smaller things, take or leave
- A test would be cheap now in a way it was not when you opened this:
integration-tests/runtime_operation_test.goalready captures the runtime-operations POST body (lines 79-89), so adding--parameter foo --parameter barto therunsubtest and assertingbody["parameters"]would pin the wire format in a few lines. That file was added 2026-05-26, after you opened this, so it was not available to you — but you will be rebasing past it anyway. A case with a value containing a space would document the intended semantics. - The example at
RunCommand.php:45uses--parameter param1 --parameter param2, which does not convey what a parameter is. Comparesource-operation:run's example (update --variable env:FOO=bar). Something likemigrate --app app --parameter --forcereads better. - Unrelated to you:
ArrayArgument::split()preserves array keys afterarray_filter(), so a leading comma or space (--parameter ",a") produces a JSON object instead of an array. It is a pre-existing helper defect that already affects other callers on main, and it becomes moot for this command once the splitting goes away. We will fix the helper separately.
Once the --parameter handling is direct and the branch is rebased, this is good to merge.
Review by Claude Code.
7c0af35 to
d2c8c31
Compare
There was a problem hiding this comment.
Note
Reviewed — No blocking findings · 🔵 1 minor point
🔍 Full review · 2 files reviewed
🔵 Minor point
legacy/src/Command/RuntimeOperation/RunCommand.php:42— The--parameteroption is declared asVALUE_REQUIRED | VALUE_IS_ARRAYand its value is forwarded verbatim ($input->getOption('parameter')) toexecRuntimeOperation(), with no splitting on commas. The PR description states the option "accepts repeated values or comma-separated lists", and the new integration test confirms the opposite:--parameter "my value,other"is sent as the single parameter"my value,other"(integration-tests/runtime_operation_test.go:137). A user who follows the PR description and writes--parameter=a,bgets one parametera,bpassed to the remote operation instead of two. Either the description/help text or the implementation needs to change; the option help ("one per option, in order") matches the code, so the description is the likely error.
Verification
- The option is registered with VALUE_REQUIRED|VALUE_IS_ARRAY, so getOption('parameter') always returns an array (empty when unset) for the third argument of execRuntimeOperation.
- Passing parameters as argument #3 keeps the existing argument order ($operationName, $appName) untouched, and the legacy-php job (php-cs-fixer + phpstan level 8) reports pass on this head, so the client signature accepts the third array argument.
- The added subtest asserts the exact JSON body of the runtime-operations POST, and the existing "run" subtest now asserts no "parameters" key is sent when the option is omitted.
- The new option name does not collide with the project/environment/app/worker or wait options already added to the definition.
Verified by the new run_with_parameters subtest in integration-tests/runtime_operation_test.go, which asserts the POST body's parameters array, plus the pre-existing run subtest now asserting the key is absent; these run in the CI integration-test job (pending at review time), while legacy-php (php-cs-fixer, phpstan level 8, phpunit) already passes on this head. No PHP unit test covers the new option.
Review details
- Commit: d2c8c31
- Model: claude-opus-5
Review 1 of 10 for this pull request · View the full run
Allow passing parameters to runtime operations via the --parameter option, which accepts repeated values or comma-separated lists. Requires platformsh/client ^3.0.0-beta3 for the new $parameters argument in execRuntimeOperation(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The API treats each parameter as one positional argument, so values may contain spaces or commas. Splitting via ArrayArgument made those values impossible to express. Read the option directly instead, like source-operation:run --variable. Add integration test coverage for the request body, and use a more representative help example. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
d2c8c31 to
f4a90a0
Compare
Allow passing parameters to runtime operations via the repeatable
--parameteroption. Each use passes exactly one parameter, in order; values are not split on commas or whitespace, so they may contain either.