Support tasks in resources read and write - #116
Conversation
composer.json temporarily references the platformsh/client task branch; revert before merge. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds support for deployment tasks to the legacy resources:get / resources:set flow, so tasks can be displayed and have their profile size updated, plus introduces --task filtering alongside existing resource filters.
Changes:
- Extend deployment service aggregation and filtering to include
Taskobjects. - Update
resources:getandresources:setto expose--taskand to handle task-specific shape differences (no disk/instance_count/type). - Update
platformsh/clientdependency to a dev branch that provides theTaskdeployment model.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
legacy/src/Service/ResourcesUtil.php |
Adds task awareness to service listing, disk support, and filtering. |
legacy/src/Command/Resources/ResourcesSetCommand.php |
Adds --task (and other name filters) and handles task-specific resource fields while building updates/summaries. |
legacy/src/Command/Resources/ResourcesGetCommand.php |
Adds --task and renders tasks safely (no type/instances). |
legacy/composer.json |
Pins platformsh/client to a dev branch providing the Task model + adds VCS repository. |
legacy/composer.lock |
Locks the updated platformsh/client source and related dependency metadata. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if ($input->hasOption('type') && ($requestedTypes = ArrayArgument::getOption($input, 'type'))) { | ||
| $byType = []; |
| ->addOption('force', 'f', InputOption::VALUE_NONE, 'Try to run the update, even if it might exceed your limits') | ||
| ->addOption('dry-run', null, InputOption::VALUE_NONE, 'Show the changes that would be made, without changing anything'); | ||
| ->addOption('dry-run', null, InputOption::VALUE_NONE, 'Show the changes that would be made, without changing anything') | ||
| ->addOption('service', 's', InputOption::VALUE_REQUIRED | InputOption::VALUE_IS_ARRAY, 'Limit to a service name. This can select any service, including apps and workers.') |
| @@ -51,6 +52,7 @@ protected function configure(): void | |||
| ->addOption('service', 's', InputOption::VALUE_REQUIRED | InputOption::VALUE_IS_ARRAY, 'Filter by service name. This can select any service, including apps and workers.') | |||
| "guzzlehttp/guzzle": "^7", | ||
| "platformsh/console-form": "^1@beta", | ||
| "platformsh/client": "^3@beta", | ||
| "platformsh/client": "dev-add-task-deployment-model as 3.0.0-beta5", |
pjcdawkins
left a comment
There was a problem hiding this comment.
Reviewed while it is still in draft, since the main point is a design question worth settling before more work goes in. The direction is right and the resources-command changes are mostly careful. The problem is narrow: widening the shared accessor changes the input of three commands that were not updated.
The design call
ResourcesUtil::allServices() now merges $deployment->tasks into the map (ResourcesUtil.php:47), but its three other callers were not touched.
AutoscalingSettingsSetCommand.php:113—validateServiceSupportsAutoscaling(string $serviceName, Service|WebApp|Worker $service, ...)at:712andtypeName(WebApp|Worker|Service $service)at:907are real PHP type declarations, soautoscaling:set --service <task-name>fails on aTask. Symfony's defaultcatchExceptions = truerenders it as an error block with exit 1 rather than a fatal, so "uncaught TypeError" would be overstating it — but the command is unusable. Interactiveautoscaling:setis worse:filterServicesWithAutoscalingSupport()returns true for tasks, so they are offered in the picker and selecting one fails at:218.ResourcesSizeListCommand.php:59is a third caller that also breaks, since$container_profileis undefined onTask.autoscaling:getdoes not hit the TypeError (plainarrayparameter) and only starts listing tasks if the autoscaling API returns a settings entry keyed by that name — so there the PR removes a guard rather than actively listing them.
Recommendation: keep allServices() narrow and make tasks opt-in — either allServices(EnvironmentDeployment $deployment, bool $includeTasks = false) with only the resources commands passing true, or a separate allServicesAndTasks(). That way the autoscaling and sizes commands keep the contract they were written against, and the PHPStan errors below resolve as a side effect rather than needing suppression.
The type property question
ResourcesUtil.php:154 reads $service->type for every entry. Task uses ReadOnlyStructureTrait, whose __get() throws Property not found: type for a missing key. Checked against a live project: the deployment tasks payload contains only container_profile and resources — no type. So resources:get --type <t> breaks on any project with tasks unless an earlier name filter already excluded them.
This PR's own comment at ResourcesGetCommand.php:132 is correct about this; it is the new client model's @property-read string $type docblock that is inaccurate and should be removed. Worth settling in the client before this rebases, since #112 and this PR currently disagree about Task's shape.
CI
legacy-php fails at make lint-phpstan (job 83248229517, exit 2, 9 errors): five signature mismatches in the autoscaling commands (AutoscalingSettingsGetCommand.php:86, AutoscalingSettingsSetCommand.php:112, :113, :197, :218), three property.notFound at ResourcesSizeListCommand.php:66/:77/:92, and one stale ignore pattern at legacy/phpstan-baseline.neon:204 still spelling the three-type union.
Please do not regenerate the baseline to clear these — three of the nine are the real runtime faults described above. Fixing the accessor design makes the lint pass on its own.
Also worth knowing: because PHPStan aborts the job, "Run PHPUnit tests" and "Build platform.phar" (.github/workflows/ci.yml:90-94) have never executed on this branch, so even the existing suite is unverified here.
Smaller items
ResourcesUtil.php:142— the name filters intersect rather than union, soresources:set --app main --task mytaskalways errors with "No tasks were found matching..." even though both exist. Pre-existing for--app+--worker, butresources:setis newly gaining all four options, so it becomes much easier to hit.ResourcesSetCommand.php:74— adds-sfor--serviceon a command that already uses-Sfor--size.resources:set -s 2XLerrors rather than doing damage, but a case-only distinction between a selector and a value-setter on a mutating command is easy to fat-finger.ResourcesUtil.php:56— theinstanceof Taskarray_filteris dead code oncetasksis registered inEnvironmentDeployment::$types, and the "older clients pass raw arrays" comment is misleading given composer.lock pins the client. Keep thegetData()guard, it is needed.ResourcesSetCommand.php:136—filterServices()returnsarray|falsebut the caller only checksempty(). It works, sinceempty(false)is true, but it conflates a filter error with an empty selection. CompareResourcesGetCommand.php:91-95.ResourcesSetCommand.php:165— the confirmation table runsresources:getwith only--project/--environment, soresources:set --task mytaskshows every app, worker, service and task while only the filtered subset will change.
Tests
None, and nothing existing to lean on — no ResourcesUtilTest.php, no legacy/tests/Command/Resources/. Both risky behaviors are cheap to cover: filterServices() is a pure function of an array plus an InputInterface, and allServices() needs only an EnvironmentDeployment fixture with and without a tasks key. Worth adding a Task::fromData([]) case with no container_profile, no type and no resources, since that is the shape the four ?? 'BALANCED' fallbacks assume.
This cannot merge yet regardless, since it depends on an unreleased branch of platformsh/client — hence flagging the design now so it can be reworked while the client release lands.
Review by Claude Code.
As a bonus, adding type filters to
resources:set, i.e.resources:set --task myagentDepends on platformsh/platformsh-client-php#107 , hence drafted for now.