feat(account): read an invoice, and say what a machine actually costs - #266
feat(account): read an invoice, and say what a machine actually costs#266Denis-hamon wants to merge 27 commits into
Conversation
…to change
`vps service-info edit myvps --renew-period 12` used to send this:
{"renew":{"automatic":false,"deleteAtExpiration":false,"forced":false,
"manualPayment":false,"period":12}}
The renewal settings are booleans bound to a shared struct carrying no
`omitempty`, so every one of them was marshalled at its zero value and won the
merge against the fetched resource. Changing the renewal period therefore also
switched automatic renewal off — on a service that had been renewing itself for
years, without a word in the output saying so.
Webhosting already built its payload from `cmd.Flags().Changed`, and did not
have the defect. This promotes that builder into `common`, so `vps` gets it too
and the next `service-info edit` cannot reintroduce the bug by reaching for the
struct.
Reading `Changed` rather than the values keeps `--renew-automatic=false`
working: pflag records a flag as changed whatever value it was given, so an
explicit false is still sent while an absent flag stays absent. Both cases are
covered by a test, and each test was checked against the failure it exists to
catch.
The shared mutable `ServiceInfoSpec` goes away with the last thing that read
it, and the five flag registrations repeated across four commands become one
call, which also settles the two spellings of the period's help text.
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
…ayer Declaring cobra flags is the command layer's job, and internal/services/common was the only service package doing it — the shared flag helpers all live in internal/cmd. Raised in review of this PR. The registration moves; the table does not. Both halves need the flag name — one to declare it, the other to read whether the operator set it — and that name is the only thing tying them together. Splitting it into two copies would mean a rename could touch one side and leave the other silently no longer sending a setting, which is the exact failure this PR exists to fix. So common.ServiceInfoRenewFlags becomes the exported description, internal/cmd registers from it, and the payload builder keeps reading it. Checked by renaming an entry in that table and watching the service-info tests fall: the two halves still move together. Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The /me schema carries 147 billing paths and the CLI exposed none of them. An operator who wanted to know what a server costs, or what the last invoice charged for, had to leave the terminal. Three measurements shaped this rather than the schema. The date window is a guard, not a convenience. The account this was built against holds 2215 invoices; a year is 396 and a month is 40. Listing expands one HTTP call per invoice, so an unbounded `bill list` is 2215 requests. Without --from the window is the current month, and past 400 matches the command says how many rather than making them. The download link is a bearer credential, and that is measured rather than assumed: a real pdfUrl returns the PDF in full with no API token at all -- 200, application/pdf, 298 KB -- and the same URL with its esign parameter altered returns HTML instead. So password, url and pdfUrl are fingerprinted unless --reveal, with the substitution applied to the object and not in the template. A mask that only covers the human-readable output covers nothing, because it is the pipeline that logs. Usage is one row per element because the entry-level price is null on every current-period entry measured, and the amount lives in the elements. A table built on the entry would have shown a column of nothing. `baremetal cost` exists because dedicated servers appear nowhere in usage -- 0 of 12 tested -- being billed flat rather than per use. Summing invoice lines for one machine costs 1109 requests for a single month; /v1/services answers in six, and answers better: a server is four or five billable services, so the price shown is the machine as configured, not its base plan. Three servers on this account carry a component billed at 70, 10 and 8 euros, which is why the components are added rather than listed for decoration. It reads renewal from /v1/services and not from serviceInfos, deliberately. Six of the 35 servers are child services -- a parentServiceId, renewalType "option", no renewal block of their own -- and on exactly those six serviceInfos returns contradictory values for renew.automatic on consecutive reads: 30 reads in 13 seconds gave 16 transitions with no convergence, every response a 200. Filed as PUBM-55135. So this reports what is true, that the parent carries the renewal, and names it, instead of deriving a boolean that would be right half the time. The billing surfaces that are empty on this account -- deposit, withdrawal, reverse bill, down payment, credit balance, billing group, purchase order, payment transaction, voucher -- are left out. Listing them is demonstrable; their details are not, on this account. Signed-off-by: Denis <denis.hamon@ovhcloud.com>
A real invoice identifier read from the account under test ended up in the refund fixture, and the server address of ovh#265 came along with the rebase. Neither is a secret and neither buys anything: replaced with invented values of the same shape, and with 203.0.113.7 from the range RFC 5737 reserves for documentation. Signed-off-by: Denis <denis.hamon@ovhcloud.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
A hostname, an address, an order id and a colleague's name read off a live account had been used as fixtures and as examples in comments. This repository is public: what goes in stays in. The values are replaced with synthetic ones of the same shape — RFC 5737 documentation addresses, hostnames built on them, identifiers of the same length — so the tests keep exercising the same parsing. This is the fourth time in this series that live account data reached a commit, after a live IP migration token, a test server's address, and a real invoice id. The first three were fixed one at a time as they were noticed; this is the sweep that should have followed the first one. Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Two OVH-generated bearer credentials read off a live account were still in test fixtures: an order-form password, which opens display-order.cgi?orderId=…&orderPassword=… with no API token — a property this very patch documents — and a bill password, one of the three fields the billing command masks by default precisely because it is a bearer secret. The account's default payment-method id went with them. The earlier sweep is what makes this worth writing down. It replaced the order ids, the invoice id and the esign signature, and left the passwords sitting next to them: in the bill fixture, orderId was already synthetic and esign was already SIGNATURE, and only `password` was still real. What names a thing was cleaned; what opens it was not. Removing a value from the tip does not remove it from a pushed commit. Both credentials must be treated as compromised regardless of this commit. Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3a76445 to
965cb5d
Compare
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
`bill list`, `refund list` and `usage` registered --filter through withFilterFlag and then called display.RenderTable directly. RenderTable does not filter; withFilterFlag only binds the flag to flags.GenericFilters. The flag was accepted by cobra, written into doc/ by docgen, and inert — an operator reading a list they believe was narrowed is worse served than one who was never offered the flag. These three now go through common.RenderFilteredTable. `baremetal cost` is the opposite case and gets the opposite fix. It renders one object through a template, so it has no rows to filter and the flag is removed rather than implemented. The test asserts the invoice the filter EXCLUDES is absent, not merely that the kept one is present: the second passes just as well with the filtering removed. Found by a review pass over this branch, then confirmed by an audit of every withFilterFlag call site in the tree. Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
FetchObjectsParallel preallocates one slot per identifier and only writes the
ones that succeeded, so with --ignore-errors the failures stay in the slice as
nil maps. ListBills and ListRefunds passed them straight through
billSecretsView, which copies from a nil map, iterates nothing, and returns a
non-nil empty map. One blank row per failed read in the table, one bare {} per
failed read under -o json — and nothing saying how many were missing, which is
worse than a short list because a short list is visible.
--ignore-errors exists to make a partial failure survivable, not to invent
entries.
FetchExpandedArray, twenty lines away in the same file, already drops them and
says so in a comment. These two call sites bypass it because they need a query
string and a pre-flight count. The same latent defect exists at around forty
other call sites of FetchObjectsParallel across baremetal, cloud, browser,
webhosting, ip and support tickets — several of them structurally identical.
Hoisting the filter into the helper is the real fix and belongs on main, not
here: one of those callers takes a slice of structs, where a nil check does not
apply at all.
One test on the row count, red under a sabotage that removes the filter.
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
ovh#243 moved the Cobra registration of the renewal flags out of internal/services/common and into the command layer, and exported the descriptor table so both halves keep reading one list. That commit landed after this branch had already taken its parent, so this branch still carried the version where a service package declares flags — and being the more recent side of the merge, it would have won and quietly undone the refactor. Merged rather than rebased: nothing is rewritten, so the review threads on this PR stay attached to their lines. Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Human review — Does not workN2 · An invoice link is a bearer secret
🛑 failed to read invoice FR000000000: failed to fetch object "FR000000000": OVHcloud API error (status code 404): "The requested object (billId = FR000000000) does not exist" (X-OVH-Query-Id: EU.ext-3.6a8b4603.1312250.9060d06909d2489ea661f458fdfb2a1c) — Denis (Product Manager), through CLI sandbox review page |
Human review — Does not workN3 · A default date window, and a refusal beyond it
🛑 2107 invoices match, which is 2107 requests to detail them. — Denis (Product Manager), through CLI sandbox review page |
|
Follow-up on N2 and N3. Both are false negatives, from two different causes. N2 — the invoice ID was fictitious. The sandbox now retargets per command instead of sending everything to one machine. N3 — the refusal is the pass. |
A --dry-run already carries the parameters in its message, and a
log.Println sitting just above the branch repeated the same JSON behind
a Go timestamp no other command in this CLI emits:
🔍 Dry run: nothing was sent. This would have been posted to …
{ "operatingSystem": "debian12_64" }
2026/08/23 23:47:22 Final parameters:
{ "operatingSystem": "debian12_64" }
The log line moves below the branch. A real run still logs what it is
about to send, which is what it was for; a dry run logs nothing, because
it sends nothing.
The line goes to stderr, so no assertion on stdout could ever have seen
it — which is why it survived every green run. Two tests now redirect the
logger: one that a dry run does not log, and its positive control that a
real run still does, so deleting the line outright would not pass.
Signed-off-by: Denis <denis.hamon@ovhcloud.com>
Mechanical, not a change of behaviour. Placing them just after TestBaremetalReinstallDryRun put them in the one region of baremetal_test.go that 23 downstream branches also append to, and in the import block they all touch as well. Merging the parent into those branches failed 23 times out of 23, every one of them on adjacency rather than on a disagreement. baremetal_test.go goes back to what it was before the previous commit, so this branch now leaves that file untouched. A new file can only clash with a file of the same name, and nothing else carries this one. Signed-off-by: Denis <denis.hamon@ovhcloud.com>
…' into feat/account-billing
Signed-off-by: Denis <denis.hamon@ovhcloud.com>
Human review — Works as intendedN3 · A default date window, and a refusal beyond it
🛑 2108 invoices match, which is 2108 requests to detail them. — Denis (Product Manager), through CLI sandbox review page |
Stacked on #265. The reviewable unit is the last commit.
Nothing in the CLI reads an invoice. The
/meschema carries 147 billing paths and theaccountcommand exposes none of them: an operator who wants to know what a machine costs, orwhat the last invoice charged for, leaves the terminal.
This adds the reading surface, and one command that answers the question a BareMetal operator
actually has.
What the measurement decided
Measured on a real account, 20 August 2026.
The date window is a guard, not a convenience. The account holds 2215 invoices; a year is
396, a month is 40.
ManageListRequestexpands one HTTP call per item, so an unboundedbill listis 2215 requests. Without
--fromthe window is the current month, and past 400 matches thecommand says how many rather than making them.
The download link is a bearer credential, and that is measured rather than assumed. A real
invoice
pdfUrlreturns the PDF in full with no API token at all — HTTP 200,application/pdf, 298 KB. The same URL with itsesignparameter altered returns HTML instead, sothe signature is what authorises.
password,urlandpdfUrlare therefore fingerprinted bydefault and
--revealprints them, with the substitution applied to the object and not in thetemplate: a mask that only covers the human-readable output covers nothing, because it is the
pipeline that logs.
--categoryis checked against the schema, not against a list retyped here.billing.CategoryEnumholds seven values including
purchase-servers. The game protocols of #256 were copied by hand andwere already stale when they shipped.
Usage is one row per element, because the entry-level price is null. On every current-period
entry measured,
priceisnulland the amount lives inelements— 22 of 25 entries carryelements. A table built on the entry would have shown a column of nothing. The forecast fills both,
so the entry price is the fallback.
Dedicated servers do not appear in usage at all — 0 of 12 servers tested. They are billed at a
flat rate, not per usage, which is why
baremetal costexists and reads somewhere else.baremetal cost, and why it does not read invoicesSumming invoice lines for one machine costs 1109 requests for a single month (40 invoices,
1068 detail lines, none of them filterable by service).
/v1/services?resourceName=<server>answersthe same question in six.
It also answers it better. A server is not one billable service but four or five: the machine,
and the components sold with it. So the price shown is the price of the machine as configured:
Renewal is read here rather than from
serviceInfos, deliberately. Six of the 35 servers on theaccount are child services — they carry a
parentServiceId,renewalType: option, and no renewalblock of their own. On exactly those six,
GET /dedicated/server/{sn}/serviceInfosreturnscontradictory values for
renew.automaticon consecutive reads: 30 reads in 13 seconds gave 16transitions, with no convergence, every response HTTP 200. The 29 servers without a parent are
stable 20/20, and
/v1/servicesis stable 15/15 everywhere including the six. Filed upstream asPUBM-55135. So this command reports what is true — the parent carries the renewal — and names
the parent, instead of deriving a boolean that would be right half the time.
Verification
Gate green on the five steps:
go build,go vet,make wasm,go test ./..., docgen.23 tests (18 unit, 15 cobra) and 15 sabotages, 15 red, with restoration verified by
checksum and
go test -count=1after each repair.Every command was run against the real API: the default window, the category filter, the volume
guard, the masking under
-o json(the point that matters),--reveal, the details of a realinvoice, the refunds, both usage routes, and
coston both a normal server and a child service.Every command named in an error message was checked against the binary's own command tree.
Not in this patch
The billing surfaces that are empty on this account — deposit, withdrawal, reverse bill, down
payment, credit balance, billing group, purchase order, payment transaction, voucher — all return
[], anddebtAccountandfidelityAccountreturn 404. Listing them is demonstrable; theirdetails are not, on this account. Same reason
burstwas left out of #254.Payment methods and the order surface (
/me/order, 3625 orders) are their own lot.