Skip to content

feat(resources:set): allow --count for horizontally scalable services - #185

Open
pjcdawkins wants to merge 4 commits into
mainfrom
cli-146-add-manual-horizontal-scaling-for-db-replica-services-in-cli
Open

pjcdawkins wants to merge 4 commits into
mainfrom
cli-146-add-manual-horizontal-scaling-for-db-replica-services-in-cli

Conversation

@pjcdawkins

Copy link
Copy Markdown
Contributor

Services such as database replicas can be scaled manually through the sizing API, but resources:set rejected any instance count for a service:

Error in --count value:
  The instance count of the service database_replica cannot be changed.

This allows --count (and the interactive prompt) for a service when the deployment reports supports_horizontal_scaling: true for it. Other services get a "does not support horizontal scaling" error. Tasks, the autoscaling-enabled check, and the instance limit are unchanged.

🤖 Generated with Claude Code

Services such as database replicas can be scaled manually through the
sizing API, but the CLI rejected any instance count for a service.

Allow the instance count of a service when the deployment reports
supports_horizontal_scaling for it, in both --count and the interactive
form. Services without the flag now get a "does not support horizontal
scaling" error. Tasks and the autoscaling check are unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@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

🔍 Full review · 2 files reviewed

Verification
  • validateInstanceCount still rejects a Task before the new horizontal-scaling check, so tasks keep the "cannot be changed" message.
  • For a scalable service, the autoscaling-enabled check and the instance limit check still run after the new service check.
  • The execute loop and validateInstanceCount both use supportsInstanceCount, so --count and the interactive prompt apply the same rule to services.
  • computeMemoryCPUStorageDiff already multiplies CPU and memory by instance count for any group, so trial-limit checks cover service count changes.

The new testValidateInstanceCount in ResourcesSetTest.php covers validateInstanceCount for apps, workers, tasks and services with and without the flag, including the autoscaling and limit cases. It runs in the legacy-php CI job (PHPUnit, plus phpstan and php-cs-fixer). No test covers the execute-loop path for services, including the interactive prompt and the update payload.

Review details
  • Commit: 151c792
  • Model: claude-opus-5-5

Review 1 of 10 for this pull request · View the full run

Comment thread legacy/src/Command/Resources/ResourcesSetCommand.php
Comment thread legacy/src/Command/Resources/ResourcesSetCommand.php
A service may have no instance_count. The interactive form then compared
the accepted default of 1 against null and queued a no-op update.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@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 found · 1 still open

🔁 Incremental · 1 file reviewed

Outstanding from earlier reviews:

  • 🟡 #4117201704 — legacy/src/Command/Resources/ResourcesSetCommand.php:560: Users may be allowed to request scaling that the project cannot perform. — supportsInstanceCount still checks only the deployment's supports_horizontal_scaling flag and not the project capability. The author says this is intentional because the API does not gate manual sizing on that capability. The disagreement with autoscaling:set remains.
Verification
  • $currentCount = $properties['instance_count'] ?? 1 is read once. The --count comparison, the prompt default and the interactive comparison all use it, so a missing key no longer triggers an undefined-key warning.
  • Accepting the default '1' for a service with no instance_count now compares int 1 to int 1, so no update is queued. validateInstanceCount returns an int, which keeps the strict !== comparison correct.
  • The simplified --count check $instanceCount !== $currentCount behaves the same as the old compound condition that special-cased an unset count with a requested count of 1.

The diff adds 35 lines to legacy/tests/Command/Resources/ResourcesSetTest.php for the service count path. I read the command change and the QuestionHelper/validator types, but not the new test bodies. I did not run the tests.

Review details

Review 2 of 10 for this pull request · View the full run

pjcdawkins and others added 2 commits September 27, 2026 23:20
Add integration tests with a deployment holding an app, two workers
(one autoscaled), three services (one horizontally scalable with no
instance_count, one without a disk) and a task.

They check the PATCH body or errors for --count, --size, --disk and
--object-storage on each container type, wildcards, the instance limit,
autoscaling, unknown containers, --service filtering and --dry-run, and
that the interactive form asks for a count only for a scalable service.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Add to mockapi:
- Environment.SetNextDeployment, serving the next deployment as raw data
  with self and #edit links, and recording PATCH bodies, which
  Handler.DeploymentPatches returns
- Environment.SetAutoscalingSettings, serving the settings and adding
  the #autoscaling and #manage-autoscaling links
- Project.Settings, served at the project's /settings path

Use them in the resources and autoscaling integration tests, and merge
the three resources setup helpers into setUpResourcesProject, with
nextDeployment, serveAPI and deploymentPatch helpers.

Remove the organization profile stubs: resources:set no longer requests
the profile.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@upsun-dispatch

Copy link
Copy Markdown

📋 PR Summary

This PR lets resources:set change the instance count of a service, through --count or the interactive prompt, when the deployment reports supports_horizontal_scaling: true for it. Any other service now fails with a "does not support horizontal scaling" error. It also adds built-in routes to the mock API for next deployments, autoscaling settings and project settings. The resources and autoscaling integration tests now use these routes and a few shared setup helpers.

Changes
Layer / File(s) Summary
Command behavior
legacy/src/Command/Resources/ResourcesSetCommand.php Accepts an instance count for services that support horizontal scaling, and rejects it for other services with a specific error.
legacy/tests/Command/Resources/ResourcesSetTest.php Adds unit tests for setting the instance count on a service that supports horizontal scaling and on one that does not.
Mock API
pkg/mockapi/api_server.go Adds routes for project settings, GET and PATCH on the next deployment, and autoscaling settings.
pkg/mockapi/environments.go Adds handlers for these routes. It records each PATCH to the next deployment and makes them available through DeploymentPatches, and adds self and edit links to the responses.
pkg/mockapi/model.go Adds a Project.Settings field, plus SetNextDeployment and SetAutoscalingSettings. The autoscaling setter also adds the environment's autoscaling links.
pkg/mockapi/projects.go Serves Project.Settings at the project's /settings path.
Test helpers
integration-tests/resources_helpers_test.go New shared helpers: setUpResourcesProject, nextDeployment, serveAPI, deploymentPatch and patchedApp.
Resources tests
integration-tests/resources_set_containers_test.go Tests resources:set options across apps, workers, services (including a replica that supports horizontal scaling) and tasks, using the shared helpers and the recorded patches.
integration-tests/resources_set_test.go Now uses the shared setup helpers.
integration-tests/resources_set_values_test.go Replaces the local setup and PATCH capture with the shared helpers.
integration-tests/resources_set_interactive_test.go Now uses the shared helpers.
integration-tests/resources_set_trial_test.go Setup now uses setUpResourcesProject and serveAPI.
integration-tests/resources_get_test.go Now uses the shared helpers.
integration-tests/resources_sizing_disabled_test.go Sets project settings through the new Settings field instead of a custom route.
Autoscaling tests
integration-tests/autoscaling_enabled_test.go Uses SetAutoscalingSettings instead of hand-written links and a GET route.
integration-tests/autoscaling_missing_defaults_test.go Uses SetAutoscalingSettings.
integration-tests/autoscaling_new_service_test.go Uses SetAutoscalingSettings.
integration-tests/autoscaling_settings_set_test.go Uses SetAutoscalingSettings.
integration-tests/autoscaling_validate_metric_test.go Uses SetAutoscalingSettings.

This branch has not been deployed

No deployments
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.

1 participant