Skip to content

Support tasks in resources read and write - #116

Merged
bojanz merged 7 commits into
mainfrom
resources-task-containers
Aug 7, 2026
Merged

Support tasks in resources read and write#116
bojanz merged 7 commits into
mainfrom
resources-task-containers

Conversation

@vitolkachova

@vitolkachova vitolkachova commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

As a bonus, adding type filters to resources:set, i.e. resources:set --task myagent

Depends on platformsh/platformsh-client-php#107 , hence drafted for now.

Copilot AI review requested due to automatic review settings June 24, 2026 16:33

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Task objects.
  • Update resources:get and resources:set to expose --task and to handle task-specific shape differences (no disk/instance_count/type).
  • Update platformsh/client dependency to a dev branch that provides the Task deployment 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.

Comment thread legacy/src/Service/ResourcesUtil.php
Comment thread legacy/src/Command/Resources/ResourcesSetCommand.php Outdated
Comment thread legacy/src/Command/Resources/ResourcesGetCommand.php Outdated
Comment thread legacy/composer.json Outdated
@vitolkachova
vitolkachova marked this pull request as draft June 24, 2026 16:41

@pjcdawkins pjcdawkins left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:113validateServiceSupportsAutoscaling(string $serviceName, Service|WebApp|Worker $service, ...) at :712 and typeName(WebApp|Worker|Service $service) at :907 are real PHP type declarations, so autoscaling:set --service <task-name> fails on a Task. Symfony's default catchExceptions = true renders it as an error block with exit 1 rather than a fatal, so "uncaught TypeError" would be overstating it — but the command is unusable. Interactive autoscaling:set is worse: filterServicesWithAutoscalingSupport() returns true for tasks, so they are offered in the picker and selecting one fails at :218.
  • ResourcesSizeListCommand.php:59 is a third caller that also breaks, since $container_profile is undefined on Task.
  • autoscaling:get does not hit the TypeError (plain array parameter) 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, so resources:set --app main --task mytask always errors with "No tasks were found matching..." even though both exist. Pre-existing for --app + --worker, but resources:set is newly gaining all four options, so it becomes much easier to hit.
  • ResourcesSetCommand.php:74 — adds -s for --service on a command that already uses -S for --size. resources:set -s 2XL errors 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 — the instanceof Task array_filter is dead code once tasks is registered in EnvironmentDeployment::$types, and the "older clients pass raw arrays" comment is misleading given composer.lock pins the client. Keep the getData() guard, it is needed.
  • ResourcesSetCommand.php:136filterServices() returns array|false but the caller only checks empty(). It works, since empty(false) is true, but it conflates a filter error with an empty selection. Compare ResourcesGetCommand.php:91-95.
  • ResourcesSetCommand.php:165 — the confirmation table runs resources:get with only --project/--environment, so resources:set --task mytask shows 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.

@vitolkachova
vitolkachova marked this pull request as ready for review August 7, 2026 13:17
@vitolkachova
vitolkachova force-pushed the resources-task-containers branch from fabfc41 to 35f9eb4 Compare August 7, 2026 13:18
@upsun-dispatch

upsun-dispatch Bot commented Aug 7, 2026

Copy link
Copy Markdown

📋 PR Summary

This PR adds task (Task deployment model) support to the resources read/write commands and introduces a --task type filter for resources:set/resources:get. This incremental round is a small set of follow-up fixes: it guards the profile-size lookup in resources:get against tasks that have no resources.profile_size (avoiding PHP undefined-key warnings), and changes the --type filter to read the service type from properties and skip services without one rather than special-casing Task.

Changes
Layer / File(s) Summary
resources read
legacy/src/Command/Resources/ResourcesGetCommand.php Extracts $profileSize via ?? null before the isset() so a task with no resources.profile_size no longer triggers eager undefined-key access warnings.
resources filtering
legacy/src/Service/ResourcesUtil.php The --type filter now reads the type from getProperties() and skips services without a string type, instead of hard-skipping Task and reading $service->type.
dependencies
legacy/composer.lock Bumps the Composer plugin-api-version from 2.6.0 to 2.9.0 (regenerated lock metadata).

@upsun-dispatch upsun-dispatch Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 records plugin-api-version downgraded from 2.9.0 to 2.6.0 and platform-dev changed 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

Comment thread legacy/src/Command/Resources/ResourcesSetCommand.php
Comment thread legacy/src/Command/Resources/ResourcesGetCommand.php Outdated

@upsun-dispatch upsun-dispatch Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

Reviewed — No new issues · 2 still open

🔁 Incremental · 3 files reviewed

Outstanding from earlier reviews:

  • #3736051300legacy/src/Command/Resources/ResourcesSetCommand.php:213: Setting a new task size on a trial account floods output with PHP warnings or crashes.
  • #3736051303legacy/src/Command/Resources/ResourcesGetCommand.php:145: Displaying a task without a profile size prints PHP warnings on a normal read path.

vitolkachova and others added 5 commits August 7, 2026 15:54
composer.json temporarily references the platformsh/client task branch; revert before merge.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@vitolkachova
vitolkachova force-pushed the resources-task-containers branch from 7a6ce20 to cabcbec Compare August 7, 2026 13:54

@upsun-dispatch upsun-dispatch Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 $current build, summarizeChangesPerService(), and validateProfileSize(). 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:

  • #3736051300legacy/src/Command/Resources/ResourcesSetCommand.php:213: Setting a new task size on a trial account floods output with PHP warnings or crashes.
  • #3736051303legacy/src/Command/Resources/ResourcesGetCommand.php:145: Displaying a task without a profile size prints PHP warnings on a normal read path.

Comment thread legacy/src/Service/ResourcesUtil.php
vitolkachova and others added 2 commits August 7, 2026 16:07

@upsun-dispatch upsun-dispatch Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

Reviewed — No new issues · 2 still open

🔁 Incremental · 3 files reviewed

Outstanding from earlier reviews:

  • #3736051300legacy/src/Command/Resources/ResourcesSetCommand.php:213: Setting a new task size on a trial account floods output with PHP warnings or crashes.
  • #3736282761legacy/src/Service/ResourcesUtil.php:63: A sibling read command breaks for exactly the profile-less tasks this PR introduces.

@bojanz
bojanz self-requested a review August 7, 2026 14:24
@bojanz
bojanz merged commit f0db42d into main Aug 7, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants