Support tasks in resources read and write - #116
Conversation
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.
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.
fabfc41 to
35f9eb4
Compare
|
📋 PR Summary This PR adds task ( Changes
|
There was a problem hiding this comment.
Warning
Changes suggested — 🟡 2 warnings · 2 minor points
🔍 Full review · 5 files reviewed
🔵 Minor points
Not blocking, and no threads opened for these.
legacy/src/Command/Resources/ResourcesGetCommand.php:124— The default task container profile'BALANCED'is hardcoded in four places across two files: here, and in ResourcesSetCommand.php at the execute loop (line 212), summarizeChangesPerService (line 461), and validateProfileSize (line 656). These copies must be changed in lockstep; if the default task profile ever changes and one copy is missed, the get/set/summarize/validate paths disagree about a task's profile.legacy/composer.lock:6344— The lock now recordsplugin-api-versiondowngraded from 2.9.0 to 2.6.0 andplatform-devchanged from{}to[], which indicates it was regenerated with an older Composer than the base branch used. This drift is unrelated to task support and will churn back the next time the lock is regenerated with a current Composer.
Review details
- Commit: 35f9eb4
- Model: claude-opus-4-8
- Panel: correctness · robustness · design
There was a problem hiding this comment.
Note
Reviewed — No new issues · 2 still open
🔁 Incremental · 3 files reviewed
Outstanding from earlier reviews:
- #3736051300 —
legacy/src/Command/Resources/ResourcesSetCommand.php:213: Setting a new task size on a trial account floods output with PHP warnings or crashes. - #3736051303 —
legacy/src/Command/Resources/ResourcesGetCommand.php:145: Displaying a task without a profile size prints PHP warnings on a normal read path.
composer.json temporarily references the platformsh/client task branch; revert before merge. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BhxKwNnEqiy6VJbZw8KPSL
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BhxKwNnEqiy6VJbZw8KPSL
…or tasks Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BhxKwNnEqiy6VJbZw8KPSL
7a6ce20 to
cabcbec
Compare
There was a problem hiding this comment.
Warning
Changes suggested — 🟡 1 warning · 1 minor point · 2 still open
🔍 Full review · 8 files reviewed
🔵 Minor points
Not blocking, and no threads opened for these.
legacy/src/Command/Resources/ResourcesGetCommand.php:124— The task container-profile default'BALANCED'is hardcoded and duplicated across four call sites: here ($service instanceof Task ? 'BALANCED' : null), and ResourcesSetCommand.php at the$currentbuild,summarizeChangesPerService(), andvalidateProfileSize(). If the platform's default task profile ever changes, all four copies must be updated in lockstep and one is easy to miss.
Review details
- Commit: cabcbec
- Model: claude-opus-4-8
- Panel: correctness · robustness · design
Outstanding from earlier reviews:
- #3736051300 —
legacy/src/Command/Resources/ResourcesSetCommand.php:213: Setting a new task size on a trial account floods output with PHP warnings or crashes. - #3736051303 —
legacy/src/Command/Resources/ResourcesGetCommand.php:145: Displaying a task without a profile size prints PHP warnings on a normal read path.
…ved type Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BhxKwNnEqiy6VJbZw8KPSL
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BhxKwNnEqiy6VJbZw8KPSL
There was a problem hiding this comment.
Note
Reviewed — No new issues · 2 still open
🔁 Incremental · 3 files reviewed
Outstanding from earlier reviews:
- #3736051300 —
legacy/src/Command/Resources/ResourcesSetCommand.php:213: Setting a new task size on a trial account floods output with PHP warnings or crashes. - #3736282761 —
legacy/src/Service/ResourcesUtil.php:63: A sibling read command breaks for exactly the profile-less tasks this PR introduces.
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.