Support for Tasks functionality - #112
Conversation
106af47 to
f9d1f47
Compare
| // when several activities are running in parallel. | ||
| $activity = reset($activities); | ||
| $this->stdErr->writeln(sprintf( | ||
| 'To follow its log, run: <info>%s activity:log %s</info>', |
There was a problem hiding this comment.
We could directly follow the log here, using the ActivityMonitor service. I think it would be more consistent with other commands.
There was a problem hiding this comment.
I deliberately decided not to as task execution can last for many hours, and IMO if a user wants to follow the log it should be an action from their side
pjcdawkins
left a comment
There was a problem hiding this comment.
Reviewed. Structurally this is in good shape: the raw authenticated Guzzle requests to GET/POST {environment}/tasks[/{name}/run] follow an established pattern (EnvironmentDeployTypeCommand.php:99-103), the --variable wire format matches the client's runSourceOperation(), and command auto-registration, the tasks alias, the environment.task activity type and URL encoding of the task name all check out. No security issues in the request handling. All four checks pass, though the branch is behind main.
One item is worth changing before merge; the rest is polish or pre-existing.
Worth changing: no way to wait
TaskRunCommand.php:95 returns 0 unconditionally once the trigger POST succeeds, so a task whose activity subsequently fails is indistinguishable from one that succeeded. There is also no --wait, because configure() (lines 31-43) never calls ActivityMonitor::addWaitOptions() and ActivityMonitor is not injected (line 26).
The practical consequence is CI: upsun task:run migrate -e main exits 0 and the pipeline reports success even when the migration failed on the container. Request-level failures (unknown task, permissions) do still exit nonzero via the rethrow at lines 62-64, so this is specifically about the activity outcome.
Every sibling that triggers an activity handles this, including the raw-HTTP one — EnvironmentDeployTypeCommand.php:105-109 guards with if ($result->getActivities() && shouldWait($input)) and returns 1 on failure. That is the pattern that applies directly here, and this command already holds the activity it would need (lines 66-72, 81-86), currently using it only to print an activity:log hint.
To be clear on the design: the objection that tasks can run for hours is reasonable, and it argues for an opt-in --wait rather than waiting by default. A single flag plus return $this->activityMonitor->waitMultiple(...) ? 0 : 1; keeps fire-and-forget as the default while making the command scriptable.
Worth a look: production runs are not confirmed
TaskRunCommand.php:47 uses getSelection($input) with the default SelectorConfig, which auto-detects the environment from the current Git branch. So upsun task:run migrate in a repo checked out on the production branch triggers the task on production with no prompt. operation:run (RuntimeOperation/RunCommand.php:120-122) and environment:redeploy (EnvironmentRedeployCommand.php:49) both confirm first; source-operation:run does not, so precedent is genuinely mixed and this is a judgement call rather than a defect.
Separately, both commands omit chooseEnvFilter: SelectorConfig::filterEnvsMaybeActive(), which the operation commands pass (RuntimeOperation/ListCommand.php:54). Without it the interactive picker offers inactive environments, where the tasks endpoint cannot work.
Polish
TaskRunCommand.php:34—taskis REQUIRED and never looked up, so a typo yields a raw GuzzleClientExceptionrather than the "task not found ... To list tasks, run:upsun tasks" form used bySourceOperation/RunCommand.php:74-80andRuntimeOperation/RunCommand.php:100-110. Behavior is correct (task not run, nonzero exit), so this is consistency, not a bug. Worth noting a fix costs an extraGET {env}/tasksper run, unlike the siblings whose lists are already in memory.TaskRunCommand.php:105—parseVariables()is copied verbatim fromSourceOperation/RunCommand.php:113-123. Moving it toPlatformsh\Cli\Model\Variableputs it under the existingVariableTest.phpand means any future parsing fix lands once.TaskRunCommand.php:70—Activity::classis passed asResult's resource class, but that argument is the class of the embedded entity;Result::setResourceClass()only affectsgetEntity(), andgetActivities()always buildsActivityregardless. Harmless today sincegetEntity()is never called. CompareEnvironmentDeployTypeCommand.php:103, which passesSettings::class.TaskRunCommand.php:63andTaskListCommand.php:50—ApiResponseException::create()is inherited fromRequestException::create()and returns a plainClientExceptionwith Guzzle's own message, so it does not add the[message]/[detail]enrichment. If the intent of that try/catch was better error messages,wrapGuzzleException()is the one that delivers them. Thecreate()form is already used at around 8 existing call sites, so this is optional and arguably a repo-wide cleanup rather than yours.
Pre-existing, not yours to fix here
Model/Variable.php:22 cannot express a variable value containing =: the regex excludes = from both the name and value groups, so env:TOKEN=abc==, env:FOO=a=b and env:URL=https://x/?a=1 all throw "Variables must be defined as type:name=value." — misleading in this specific case. This is 2019-era behavior in a file this PR does not touch, and your own example (env:CALLER=me) parses fine; task:run is just the second consumer. Relaxing group 3 to (.*) fixes it and still satisfies every existing assertion in VariableTest.php, but it also changes source-operation:run's accepted input, so it wants its own commit and test.
Tests
None added. Two seams are realistically testable with the existing harness: parseVariables() if moved to the model, and row building / command truncation for task:list if extracted (RuntimeOperation/ListCommand.php:126-133 shows truncateCommand() as the precedent). The execute() methods are not easily testable — the repo has no mock-API harness for commands — so leaving those uncovered is consistent with the codebase.
Suggest merging this first of the three Tasks PRs; it is independently useful and #113 and #116 read better against it.
Review by Claude Code.
Uh oh!
There was an error while loading. Please reload this page.