Repository navigation
feat: name disks after the API's storage and instance disk fields - #304
pjcdawkins wants to merge 4 commits into
Conversation
![upsun-dispatch[bot]](https://avatars.githubusercontent.com/in/4153575?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
Changes suggested — 🟡 1 warning · 🔵 1 minor point
🔍 Full review · 19 files reviewed
Verification
--diskand--storagetogether exit with 1 before selection; otherwise--diskis copied intostorageand its deprecation warning comes fromIo::warnAboutDeprecatedOptions.- The
@storage/@instancekeys use lenient value extraction, while the deprecated/mntcolumns keep the old strict extraction, so their old values are kept. showInstanceDiskonly looks at apps and workers whose names appear in the returned data points, so instance mounts of filtered-out apps do not show the columns.app:listreplaces thediskcolumn withstorageon flexible projects and withinstance_diskotherwise, which matches where each value comes from.
New integration tests cover the renamed columns, the deprecated aliases and the default-column logic: TestMetricsDisks, TestMetricsDeprecatedColumnsHidden, TestResourcesGet_Disks, TestAppListDisks and the new resources:set cases. They run in CI's Go integration test jobs. No test covers containers that appear in the metrics but not in the current deployment.
Review details
- Commit: 4f72730
- Model: claude-opus-5-5
Review 1 of 10 for this pull request · View the full run
CLI buildsBuilt from 28c068c (merged into the base branch). Downloads need a GitHub login, and expire after 7 days.
Or download one with the GitHub CLI, e.g.: gh run download 42252559227 -R upsun/cli -n cli-linux-amd64
chmod +x upsun
./upsun --versionOn macOS, if you downloaded it with a browser, remove the quarantine attribute before running it: |
There was a problem hiding this comment.
Note
Reviewed — No new issues found · 1 still open
🔁 Incremental · 2 files reviewed
Outstanding from earlier reviews:
- 🟡 #4474614169 —
legacy/src/Command/Metrics/MetricsCommandBase.php:519: For any container that is not in the current deployment, disk usage is silently missing from the default metrics output. — Not changed in this push:mapDisks()still getsisService()from the current deployment only, so containers missing from it get blank storage_/instance_ columns. The author chose this on purpose.
Verification
parseSetting()readsArrayArgument::getOption($input, $optionName)and puts$optionNamein its error headers, so passing$storageOption = 'disk'gives "Error in --disk value:".- The
--disk/--storageconflict check still returns before$storageOptionis used, so the two options can never be merged silently. - The
$showCompleteFormcheck now reads$storageOption, so--diskalone still skips the interactive form, as--storagedoes. - After this change, no other code in ResourcesSetCommand reads the
storageoption, so dropping thesetOption('storage', …)copy leaves no path that misses--diskvalues.
The new invalid deprecated --disk case in TestResourcesSet_Containers (integration-tests/resources_set_containers_test.go) checks the "Error in --disk value:" message. The existing deprecated --disk and -D cases cover the success path. These are integration tests, run by CI's integration-test jobs.
Review 2 of 10 for this pull request · View the full run
There was a problem hiding this comment.
Note
Reviewed — No new issues found · 1 still open
🔁 Incremental · 2 files reviewed
Outstanding from earlier reviews:
- 🟡 #4474614169 —
legacy/src/Command/Metrics/MetricsCommandBase.php:519: For any container that is not in the current deployment, disk usage is silently missing from the default metrics output. — Unchanged, and the author has explained why:isService()still reads only the current deployment, andmapDisks()maps nothing fornull. So the default storage and instance columns are blank for a container that is no longer deployed.
Verification
$defaultColumnsand theshowInstanceDisk()removal list now name onlyinstance_disk_percent, so the table never drops a default column that the list does not contain.storage_inodes_percent,instance_inodes_percentandtmp_inodes_percentare still keys inTABLE_HEADER, so--columnsand the*inodes_percentwildcard still return them. TestMetricsDeprecatedColumnsHidden checks this.- The updated CSV and table header expectations in TestMetricsDisks list the same columns, in the same order, as the new
$defaultColumnslist.
Covered by the changed TestMetricsDisks cases ("all table hides/shows instance disk", "all csv always includes instance disk") in integration-tests/metrics_storage_test.go, which run in CI's integration-test jobs. TestMetricsDeprecatedColumnsHidden covers access to inode columns through a wildcard.
Review 3 of 10 for this pull request · View the full run
880d61e to
c19501d
Compare
"Disk" meant different disks in different commands: resources:get
showed an app's network storage as "Disk", while metrics showed its
local /mnt (the instance disk). Use the names of the API's
resources.disk fields instead: storage, instance disk, temporary disk
and object storage.
Below, a column's "label" is its header in output, and its "machine
name" is how --columns selects it.
Changes:
- metrics:all and metrics:disk-usage choose disk columns by container
type, from the deployment. storage_* is a service's /mnt, or an app's
or worker's network storage. instance_disk_* and instance_inodes_*
(metrics:all), or instance_* (metrics:disk-usage), is an app's or
worker's /mnt. Storage columns are always default columns. Instance
disk and object storage columns are default columns in
machine-readable formats, and in tables when a shown container uses
them.
- metrics:all no longer shows inode percentages by default, so that
the table stays narrow. They are still available with --columns, and
metrics:disk-usage still shows them by default.
- resources:get: the disk column's machine name is now storage, and its
label "Storage (MB)". A new instance_disk column shows
resources.disk.instance for apps and workers with instance mounts.
The instance_disk and object_storage columns are hidden in tables
when empty, but always included in machine-readable formats, so the
columns are the same for every project. They are the last default
columns, after instance_count, so that scripts reading columns by
position still work (except for object_storage, which was before
instance_count on projects with object storage).
- resources:set: --storage replaces --disk, and the help, examples,
prompts and errors say "storage".
- service:list: the disk column's machine name is now storage, and its
label "Storage (MiB)".
- app:list: the disk column is replaced by storage and instance_disk.
An app's top-level disk is its storage with flexible resources, and
its instance disk on Fixed projects (from the deployment's
project_info).
- Remove the api.metrics_storage setting: storage columns now include
services' /mnt, so they can no longer be turned off.
Compatibility:
Old machine names are kept as permanent deprecated aliases, each with
a warning on stderr.
- resources:get, service:list and app:list: --columns disk selects the
new column, which has the same values (instance_disk for app:list on
Fixed).
- resources:set: --disk (and -D) is a hidden option with the same
meaning. It cannot be combined with --storage, and its validation
errors name --disk.
- The old metrics /mnt columns cannot become aliases, as their values
are now split between storage and instance disk columns. They are
kept as hidden columns with their old values and labels, added only
when named in --columns (Table::requestedDeprecatedColumns()). They
are not listed in help or matched by wildcards, except by a wildcard
that matches no other column (e.g. disk_*). A wildcard that matches
other columns (e.g. *percent) outputs a notice naming the deprecated
columns that it no longer matches:
- metrics:all: disk_used, disk_limit, disk_percent, inodes_used,
inodes_limit, inodes_percent
- metrics:disk-usage: used, limit, percent, iused, ilimit, ipercent
Output without --columns changes. In resources:get and service:list,
the renamed columns have new labels (e.g. "Storage (MB)" in place of
"Disk (MB)"). In metrics:all and metrics:disk-usage, the default
columns are different columns: the old /mnt columns are replaced by
storage and instance disk columns.
Tests cover the per-row metrics mapping by container type, the default
and deprecated columns of each command, and resources:set with
--storage, --disk and -D. The mock API gains app resources and the
deployment's project_info.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
c19501d to
dd2d60f
Compare
There was a problem hiding this comment.
Warning
Changes suggested — 🟡 1 warning
🔍 Full review · 19 files reviewed
Verification
- The deprecated
--disk/-Dvalues are now parsed under the option namedisk, so the error message names the option the user passed (Error in --disk value:). - The
disk-usagereplacement text buildsstorage_%s/instance_%sfrom each deprecated key (iused→storage_iused,instance_iused), and every name it builds exists in TABLE_HEADER. extractValuetreats the new@storage/@instancekeys as partial mountpoints, so a missing metric returns null and does not throw.- No remaining code references the removed
STORAGE_MOUNTPOINT,OBJECT_STORAGE_MOUNTPOINT,storageMetricsEnabledorapi.metrics_storage.
Integration tests cover the change: TestMetricsDisks, TestMetricsDeprecatedColumnsHidden, TestResourcesGet_Disks, TestAppListDisks and the new resources:set cases (deprecated --disk/-D, conflicting options). CI runs them in its integration test jobs, and the PHPStan baseline edits are checked by the PHP lint job. No test covers an app:list deployment without project_info.
Review details
- Commit: dd2d60f
- Model: claude-opus-5-5
Review 8 of 10 for this pull request · View the full run
…rces app:list read sizing_api_enabled only from the deployment's project_info, and treated a deployment without it as Fixed. On a Flex project, the app's storage would then be shown as its instance disk, and --columns disk would be replaced by instance_disk. Use Api::supportsSizingApi(), as resources:get does, which falls back to the project's settings. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Warning
Changes suggested — 🟡 1 warning
🔁 Incremental · 2 files reviewed
Verification
Api::supportsSizingApi()returnsproject_info.settings.sizing_api_enabledwhen the deployment has it, and otherwise falls back to the project's cached/settings, which is the fallback thread #4492375660 asked for.- The new
flexible without project_infotest case gives the projectsizing_api_enabled: trueand the deployment noProjectInfo, and expects the storage column and thereplaced by storagewarning. - The
$flexiblevalue from the new call still drives bothreplaceDeprecatedColumnsand the storage/instance_disk row values, so the alias and the data cannot disagree.
This pass covers only this push. TestAppListDisks in integration-tests/app_list_test.go gains a case for a deployment without project_info, and the other app list tests now give their mock projects settings because of the new settings request. These run in CI's integration test jobs. No test covers a failed settings request.
Review 9 of 10 for this pull request · View the full run
Detecting flexible resources may need a request for the project's settings, when the deployment has no project_info. app:list made it for any columns, so a failed request (e.g. permissions or a transient error) aborted a plain app listing, which did not need it. Only detect flexible resources when a disk column (disk, storage or instance_disk) is shown. TestAppList and TestAppListEmptyLocalConfig again have projects without settings, so they cover this. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Tables showed "N/A" where a container type cannot have a value (e.g. storage for workers, instance disk for services, instances for tasks), and a blank cell where it has none. Most cells in these columns were "N/A", which made the values hard to find, and no other table uses it. Leave those cells blank, as metrics and other tables do. Machine-readable formats already had empty values. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
"Disk" meant different disks in different commands. On Flex,
resources:getshowed an app's network storage as "Disk", whilemetricsshowed the app's local/mnt(its instance disk) as "Disk". So comparing the two was misleading. This uses the names of the API'sresources.diskfields: storage, instance disk, temporary disk and object storage.Below, a column's "label" is its header in output, and its "machine name" is how
--columnsselects it.Changes
metrics:allandmetrics:disk-usagechoose disk columns by container type, from the deployment:storage_*: a service's/mnt, or an app's or worker's network storage.instance_disk_*andinstance_inodes_*(metrics:all), orinstance_*(metrics:disk-usage): an app's or worker's/mnt.instance(orlocal) mounts.metrics:allno longer shows inode percentages by default, so that the table stays narrow. They are still available with--columns, andmetrics:disk-usagestill shows them by default.resources:get: thediskcolumn is renamed: its machine name is nowstorage, and its label "Storage (MB)". A newinstance_diskcolumn showsresources.disk.instancefor apps and workers that have instance mounts. Table cells that were "N/A" (where a container type cannot have a value, e.g. storage for workers, or instances for tasks) are now blank, as in other tables. Theinstance_diskandobject_storagecolumns are hidden in tables when empty. Machine-readable formats always include them, so the columns are the same for every project. They are the last default columns, afterinstance_count, so that scripts reading columns by position still work (except forobject_storage, which was beforeinstance_counton projects with object storage).resources:set:--storagereplaces--disk, and the help, examples, prompts and errors say "storage".service:list: thediskcolumn is renamed: its machine name is nowstorage, and its label "Storage (MiB)".app:list: thediskcolumn is replaced bystorageandinstance_disk. An app's top-leveldiskis its storage with flexible resources, and its instance disk on Fixed projects (read from the deployment'sproject_info).api.metrics_storagesetting is removed: storage columns now include services'/mnt, so they (and the object storage columns from feat(metrics): show object storage usage #303) can no longer be turned off.Compatibility
Old machine names are kept as permanent deprecated aliases, each with a warning:
resources:get,service:listandapp:list:--columns diskselects the new column, which has the same values (instance_diskforapp:liston Fixed).resources:set:--disk(and-D) is a hidden option with the same meaning. It cannot be combined with--storage./mntcolumns cannot become aliases, as their values are now split between storage and instance disk columns by container type. They are kept as hidden columns with their old values and labels. They are added only when named in--columns, so they are not listed in help or matched by wildcards, except by a wildcard that matches no other column (e.g.disk_*). A wildcard that matches other columns (e.g.*percent) outputs a notice naming the deprecated columns that it no longer matches:metrics:all:disk_used,disk_limit,disk_percent,inodes_used,inodes_limit,inodes_percentmetrics:disk-usage:used,limit,percent,iused,ilimit,ipercentOutput without
--columnschanges:resources:getandservice:list: the renamed columns have new labels, e.g. "Storage (MB)" in place of "Disk (MB)".resources:getalso gains theinstance_diskcolumn, and in machine-readable formats always hasinstance_diskandobject_storageas its last columns.metrics:allandmetrics:disk-usage: the default columns are different columns. The old/mntcolumns (e.g. "Disk %" and "Inodes %" inmetrics:all, "Used %" inmetrics:disk-usage) are replaced by storage and instance disk columns, and inode percentages are no longer shown bymetrics:all.🤖 Generated with Claude Code