feat(support): open a ticket that already knows which machine it is about - #261
Draft
Denis-hamon wants to merge 24 commits into
Draft
feat(support): open a ticket that already knows which machine it is about#261Denis-hamon wants to merge 24 commits into
Denis-hamon wants to merge 24 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>
…bout
The audit asks for ticket creation "pre-filled with machine context". The
support domain was already wired — list, get, messages, reply, create — so what
was missing was not coverage: nothing joined the ticket to the server it is
about. `create` has accepted a serviceName all along, and an operator still
retyped the server name and described its state from memory.
ovhcloud baremetal ticket <server> --subject '...' --body '...'
fills product and serviceName, and appends what the machine is: commercial
range, datacenter, active boot, monitoring, running tasks, planned maintenance,
and what `baremetal doctor` reports about it. The description stays the
operator's — the collected block says what the machine is, not what is wrong.
A server that cannot be read is written into the ticket rather than stopping it.
That is the situation somebody opens a ticket for, and a support agent learns
more from "the CLI could not read this server, here is the error" than from a
ticket that was never sent.
Four of the nine support routes had no command. Three are added:
support-tickets close <id>
support-tickets reopen <id> --reason '...'
support-tickets can-be-scored <id>
Whether a ticket may be closed is a field on the ticket, not a route: the API
answers canBeScored with a request of its own but carries canBeClosed inline. So
the check costs the read `close` already makes to name the ticket in its prompt.
Setting the score is deliberately absent. The API types it as a free string in
both the embedded and the live specification, with no enum on either side, so
the accepted values cannot be established without sending real feedback about a
real support agent.
The collection also filters server-side on eleven fields and none of them was
reachable. --filter works at the other end of the wire: it drops rows the CLI
has already fetched, and this list returns identifiers that are expanded one
request per ticket. The filters are now flags — but they belong to the
collection call and to nothing else. The expansion builds one URL per ticket by
appending to the path, so a query carried over would request
"/support/tickets?status=open/12345": a path the API cannot route, for a filter
that would have looked like it worked. A test pins that, and a sabotage that
reattaches it turns red.
The three boolean filters are only sent when they were typed. All three are
meaningful in both positions — archived=false asks for the tickets that are not
archived — so a flag left alone cannot be encoded as false without inventing a
filter the operator never asked for.
9 unit tests, 19 cobra tests, 12 sabotages, 12 red.
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
This was referenced Aug 19, 2026
Denis-hamon
marked this pull request as draft
August 20, 2026 13:09
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>
`ticket create` validated --urgency against the list read from the request body's `impact` field, and wired its shell completion to the impact completer. Nothing failed, because the two fields resolve to the same enum today — the schema declares both through one component, and that component is named support.TicketImpactUrgencyEnum. So this is a latent defect, not a live one: the day the API gives urgency values of its own, --urgency would refuse them and the refusal would list impact's values as the accepted ones. Urgency is now read from the urgency field. It costs one more OnceValues and follows the schema wherever it goes. The test situation is the interesting part, and it is stated plainly rather than papered over. TestUrgencyIsReadFromTheUrgencyField cannot discriminate while the two lists are identical: it passes with either wiring. So it comes with a canary, TestImpactAndUrgencyStillCarryTheSameValues, which goes red the day they diverge — the day the first test starts being a test, and the day the wiring starts to matter. Both were proven against a schema edited to split the two: with the fix in place only the canary fails, with the fix reverted only the reader fails. Each covers what the other cannot. Found by a third-party review pass over this branch. Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The first sweep replaced only the exact spelling. A test that checks a name can be copied back without reproducing its case carried the uppercase form of a real hostname, which therefore survived — and, once its neighbour was replaced, made that test fail. A real value does not stop being one because one of its spellings was removed. 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>
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>
…s seven checks The block written into the ticket was headed "What ovhcloud baremetal doctor reports on this server:", which a support agent reads as the full verdict. The renewal check is deliberately not run here — it reads serviceInfos five times because that field disagrees with itself between reads, and five requests is not a reasonable price for one line of context in a technical ticket. Claiming doctor's name for six of its seven checks is a smaller version of the thing doctor itself was fixed for, so the heading now says which checks these are. Same block, same class: three of the checks depend on a read of their own, and a read that failed left the check silently unrun while "nothing" still printed underneath. Those are now named, and "nothing" becomes "nothing found by the checks that did run" when any were skipped. The test that guarded the feature's headline claim asserted only on "rescue" and "Monitoring" — the first appears in the Active boot line and the second is an identity field label, so both were supplied by the identity block regardless. Measured: with those assertions, deleting *both* doctor checks from the context leaves the test green. It now asserts on the findings block itself, severity and wording included. Two tests, four sabotages, four reds — two of which the old test could not see. 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>
Contributor
Author
Human review — Does not workJ1 ·
This command can write, so live mode will not run it as is. Add --dry-run: the command then describes the calls it would make, If the command has no --dry-run, it will say so itself. — Denis (Product Manager), through CLI sandbox review page |
Contributor
Author
|
Followed up on today's "Does not work" finding (J1) — the sandbox's generic write guard ("This command can write... Add --dry-run") fired because the check was run without Rebuilt this branch and replayed live against the test/integration account: The machine's state is pre-filled exactly as J1 expects. No code change needed here. |
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>
Contributor
Author
Human review — DoubtJ1 ·
🔍 Dry run: nothing was sent. This would have been created: Subject: disk failure The second disk reports read errors --- Collected automatically by the OVHcloud CLI (ovhcloud baremetal ticket) --- Checks from ovhcloud baremetal doctor, except its renewal check (not run here): — Denis (Product Manager), through CLI sandbox review page |
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/support-ticket-lifecycle
Signed-off-by: Denis <denis.hamon@ovhcloud.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #259. Only the last commit belongs to this pull request.
C8 of the audit asks for support-ticket creation "pre-filled with machine context". The support domain turned out to be already wired —
list,get,messages,reply,create— so this is not a coverage lot. What was missing is that nothing joined a ticket to the server it is about:createhas accepted aserviceNameall along, and the operator still retyped the server name and described its state from memory.What this adds
baremetal ticketfillsproductandserviceName, and appends what the machine is: commercial range, datacenter, active boot, monitoring, running tasks, planned maintenance, and whateverbaremetal doctorreports about it. The description stays the operator's — the collected block says what the machine is, not what is wrong.A server that cannot be read is written into the ticket rather than stopping it. That is the situation somebody opens a ticket for; a support agent learns more from "the CLI could not read this server, here is the error" than from a ticket that was never sent.
What the specification says, measured
The embedded schema and the live one (
eu.api.ovh.com/1.0/support.json) were compared route by route: identical, 9 routes on both sides. No staleness this time, unlike the game protocols in #256.Five of the nine routes had commands. Three of the remaining four are added here.
canBeClosedis a field,canBeScoredis a route. The same question asked twice, by two mechanisms. Soclosechecks without an extra request, using the read it already makes to name the ticket in its prompt.Setting the score is deliberately absent. The API types
scoreas a free string in both the embedded and the live specification, with no enum on either side. Establishing the accepted values would mean sending real feedback about a real support agent. Same reasoning asburstin #254.The list filtered nothing server-side
GET /support/ticketsaccepts eleven filters and none was reachable.--filterworks at the other end of the wire: it drops rows the CLI has already fetched, and this collection returns identifiers that are expanded one request per ticket.The filters belong to the collection call and to nothing else. The expansion builds one URL per ticket by appending to the path, so a query carried over would request
/support/tickets?status=open/12345— a path the API cannot route, for a filter that would have looked like it worked. A test pins that, and a sabotage that reattaches it turns red.The three booleans are only sent when they were typed. All three are meaningful in both positions —
archived=falseasks for the tickets that are not archived — so a flag left alone cannot be encoded as false without inventing a filter the operator never asked for.Noted, not fixed here:
FetchArrayfollowsX-Pagination-Cursor-Next, which is the v2 mechanism, while this v1 collection paginates bypage/pageSizein the query. The CLI sends neither and takes whatever the API defaults to. Establishing whether that truncates needs a live call against an account with enough tickets, which this branch has not been able to make.Verification
go build,go vet,make wasm,go test ./..., docgen — all green. 9 unit tests, 19 cobra tests, 12 sabotages, 12 red, restoration verified by checksum on the three touched files.Every command named in a message was checked against the real command tree of the built binary, not assumed.
The write paths are proven by
--dry-run, unit tests and cobra tests against recorded responses — not against the live API. The credentials available to this branch expired before the sequence could run. The sequence itself is written and unchanged: create a ticket on a test server, read it back, verify out of the CLI that the body carries the machine context,can-be-scored, close, verify, refuse a second close, reopen, refuse a blank reason, close again, verify the ticket is back at rest. It will be run and this section replaced with what it measured.