fix(schemas): stop a broken refresh from installing an empty schema - #262
Draft
Denis-hamon wants to merge 24 commits into
Draft
fix(schemas): stop a broken refresh from installing an empty schema#262Denis-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>
`make schemas UNIVERSE=<name>` pipes curl into jq. When the download fails
without producing an error document — an unreachable host, a proxy that answers
nothing — curl's exit code is lost to the pipe, jq turns an empty body into an
empty file and exits 0, and the mv installs it over a working schema. Measured
against a real schema and an unresolvable host:
make schemas UNIVERSE=support SCHEMAS_ROOT=https://nonexistent.invalid.ovh
-> exit 0, internal/assets/api-schemas/support.json is now 0 bytes
An empty schema embeds, builds and passes vet. It surfaces at runtime, when a
command asks it for an enumeration and answers "value of openapi must be a
non-empty string" to somebody who typed a server name.
A 404 is caught today, but by accident: jq cannot iterate over the API's error
document, so the chain stops. It still leaves the temporary file behind.
The download now goes to a file, where curl's own exit code is the one being
tested, and the document has to declare an openapi version and at least one path
before it may replace anything. Temporary files are removed on any exit.
Same two sabotages after the change: exit 6 and exit 56, schema untouched, no
leftovers.
The v2 catalogue gets a target of its own. It could not be reached from here at
all — the v2 APIs are addressed by path, not by universe name — so every v2
schema in this repository is a file placed by hand. Four of them (iam.json,
vrackservices.json, vmwareclouddirectorbackup.json,
vmwareclouddirectororganization.json) are v2 documents stored under a name with
no _v2 suffix, and `make schemas UNIVERSE=iam` answers 404: they have no refresh
path today.
make schemas-v2 API=dedicated/server NAME=baremetal_v2
It deliberately does not curate. The v2 schemas here are a hand-picked subset of
the paths the CLI exposes, and deciding what belongs in one is a judgement call,
not a transformation. It does print the maturity breakdown of what it just
pulled in, because "Internal use only" is the thing a curator has to look at and
it is invisible in a 2 MB diff.
Nothing in the build looked inside these files, so a test now reads every one of
them the way internal/openapi does. It embeds the whole directory rather than
its *.json, so a leftover from a failed refresh reaches the test too.
It found a live defect. telephony.json does not validate: two POST operations,
on .../easyHunting/.../hunting/queue/{queueId}/agent and its ovhPabx twin,
declare billingAccount and serviceName as path parameters and not queueId. The
validator reports the first one it meets, so fixing one alone changes nothing.
The document is unusable as a result, and `ovhcloud telephony edit` fails at the
schema-filtering step for every account — measured against support.json through
the same code path, which returns a path-specific error rather than a validation
one.
That is upstream and not this repository's to fix, so it is named in
knownInvalidSchemas with its reason. The test insists it still fails: the day it
is fixed, the list goes red and asks to be shortened.
Verification: build, vet, wasm, go test ./..., docgen all clean. Five sabotages,
five red — an emptied schema, a schema reduced to {}, a leftover file in the
directory, telephony removed from the known list, and a valid schema wrongly
declared invalid. Restoration checked by digest.
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
mktemp creates the temporary file 0600 and mv keeps that mode, so a refreshed schema landed with different permissions from every other file in the directory. git only tracks the executable bit, so the repository never showed it, but a local checkout ends up inconsistent with itself. Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
An embedded schema is a hand-picked subset of a catalogue, so a path the
catalogue publishes and the file omits is normal curation. The reverse is not.
baremetal.json has one commit in the whole history of this repository -- the
initial one, of 11 September 2025 -- and it has never been refreshed. Five of
its 126 paths are no longer published, two of them badged "Stable production
version", and every one probed against the live API answers 404, including
under the method it declares:
/dedicated/server/{serviceName}/install/hardwareRaidSize GET, 404
/dedicated/server/{serviceName}/install/start POST, 404
/dedicated/server/availabilities{,/raw} 404
/dedicated/server/datacenter/availabilities/raw 404
The control that settles it: POST with no Authorization header returns 401 on
a route that exists (changeContact) and 404 on these, on eu and on ca alike --
so the router does not know them, rather than the account lacking a grant.
vrack.json carries three more and telephony.json two; ip.json, me.json and
support.json carry none.
Nothing in the CLI calls these paths today, so this is a contract defect and
not a breakage. It becomes one the day a command is built on a path that has
been gone for a year, which is exactly what an embedded schema invites.
make schemas-drift lists them. It refuses to conclude from an unusable answer
rather than reporting every path as dead, and says so plainly when a file has
no drift at all.
Signed-off-by: Denis <denis.hamon@ovhcloud.com>
Denis-hamon
marked this pull request as draft
August 20, 2026 13:09
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.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>
Denis-hamon
changed the base branch from
main
to
feat/support-ticket-lifecycle
August 20, 2026 15:19
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>
Two defects in the two targets added by the previous commit, each of which made the target useless in the exact case it was written for. `schemas-v2` never printed its maturity breakdown. The jq program carried a backslash continuation inside single quotes, which is not a continuation: make hands the shell a backslash-newline the shell leaves alone, jq receives a literal backslash and dies with a syntax error. Worse than a failed target — it died AFTER the schema was downloaded, validated and installed, so the refresh succeeded, make reported failure, and the "Internal use only" count that the target's own comment calls the one thing a curator has to look at never appeared once. Reproduced: exit 3, after installation. `schemas-drift` handed out a clean bill of health on an unreadable file. A command substitution captures stdout only, so with a mistyped NAME jq's complaint went to stderr, the capture came back empty, and the empty-string test printed "no embedded path is missing from the catalogue" and exited 0. A typo bought a pass on the very check whose comment claims to have measured five 404-ing paths. Reproduced, then measured both ways after the fix: exit 1 and "could not read …" on a missing file, exit 0 and the list on a real one. Run against the live catalogue, the repaired target reports what it was built to find: baremetal.json embeds 126 paths, the catalogue publishes 123, and five are embedded and gone — /dedicated/server/availabilities, .../availabilities/raw, .../datacenter/availabilities/raw, .../install/hardwareRaidSize and .../install/start. Verified by running the recipe bodies under sh: this machine's make is Apple's and refuses to run without an Xcode licence, so the exit codes were measured without a pipe, which on macOS returns the last element's status and would have read a failure as a success. 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>
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 fix/schemas-v2-target
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
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.
What this is
The tooling prerequisite for the two v2 lots that follow. It does two things: it stops
make schemasfrom installing a broken file, and it gives the v2 catalogue a refresh target it never had.The defect, measured
make schemas UNIVERSE=<name>pipescurlintojq. When the download fails without producing an error document, curl's exit code is lost to the pipe,jqturns an empty body into an empty file and exits 0, and themvinstalls it over a working schema.support.jsonreplaced by 0 bytesAn empty schema embeds, builds and passes
vet. It surfaces at runtime: a command asks it for an enumeration and answersvalue of openapi must be a non-empty stringto somebody who typed a server name.The 404 case is caught today, but by accident —
jqcannot iterate over the API's error document, so the&&chain stops. That is luck, not a check.The download now goes to a file, where curl's own exit code is the one being tested, and the document has to declare an
openapiversion and at least one path before it may replace anything. Temporary files are removed on any exit.schemas-v2The v2 catalogue could not be reached from here at all: v2 APIs are addressed by path (
/dedicated/server,/backupServices), not by universe name. So every v2 schema in this repository is a file placed by hand.Four of them —
iam.json,vrackservices.json,vmwareclouddirectorbackup.json,vmwareclouddirectororganization.json— are v2 documents stored under a name with no_v2suffix, andmake schemas UNIVERSE=iamanswers 404. They have no refresh path today.It deliberately does not curate. The v2 schemas here are a hand-picked subset of the paths the CLI exposes, and deciding what belongs in one is a judgement call, not a transformation. It does print the maturity breakdown of what it just pulled in, because
Internal use onlyis the one thing a curator has to look at and it is invisible in a 2 MB diff.A test that reads the schemas
Nothing in the build looked inside these files. The new test reads every one of them the way
internal/openapidoes. It embeds the whole directory rather than its*.json, so a leftover from a failed refresh reaches the test too.It found a live defect
telephony.jsondoes not validate. Two POST operations — on.../easyHunting/{serviceName}/hunting/queue/{queueId}/agentand itsovhPabxtwin — declarebillingAccountandserviceNameas path parameters and notqueueId. The validator reports the first one it meets, so fixing one alone changes nothing.The document is unusable as a result, and
ovhcloud telephony editfails at the schema-filtering step for every account:The second line is the positive control: the same code path on a valid schema returns a path-specific error, not a validation one.
That is upstream and not this repository's to fix, so it is named in
knownInvalidSchemaswith its reason. The test insists it still fails: the day it is fixed, the list goes red and asks to be shortened.Two observations, not changes
Neither is touched here — both are curation decisions for the maintainers.
cloud_v2.jsoncarries two paths that the current v2 catalogue no longer has:/publicCloud/project/{projectId}/floatingIpand/floatingIp/{floatingIpId}. The CLI drives floating IPs through v1, so nothing calls them..github/copilot-instructions.mdsaysInternal use onlypaths are never curated in.cloud_v2.jsoncontains 113 operations badgedInternal use only, against 21 alpha, 16 stable and 2 beta. The rule and the file disagree today.Verification
go build,go vet,make wasm,go test ./..., docgen — all clean.Five sabotages, five red: an emptied schema, a schema reduced to
{}, a leftover file in the directory,telephonyremoved from the known list, and a valid schema wrongly declared invalid. Restoration checked by digest.One of them was first aimed wrong — patching a single telephony operation left the twin, so the test stayed green and looked like it had no teeth. Re-aimed at the branch it was meant to exercise, it goes red. The count above is the corrected run.
Stacked on #261.
Second commit: finding the paths this repository ships that no longer exist
The target above refreshes a schema. It cannot tell you that a schema needs
refreshing, and one here badly did.
baremetal.jsonhas one commit in the whole history of this repository — theinitial one, of 11 September 2025. Five of its 126 paths are no longer published by
the catalogue, and every one probed against the live API answers 404, including
under the method it declares:
/dedicated/server/{serviceName}/install/hardwareRaidSize/dedicated/server/{serviceName}/install/start/dedicated/server/availabilities/dedicated/server/availabilities/raw/dedicated/server/datacenter/availabilities/rawThe control that settles it: POST with no
Authorizationheader returns 401on a route that exists (
changeContact) and 404 on these, oneuand oncaalike. The router does not know them — this is not an account lacking a grant. And
the same request to a path invented on the spot returns the identical body, word for
word.
vrack.jsoncarries three more,telephony.jsontwo;ip.json,me.jsonandsupport.jsoncarry none. The file also badgesreinstallBeta while it answers,and
install/startStable production version while it does not — which is why thisdoes not look like a production snapshot that merely went stale.
Nothing in the CLI calls those paths today, so this is a contract defect rather than
a breakage. It becomes one the day a command is built on a path that has been gone
for a year, which is exactly what an embedded schema invites.
The target refuses to conclude from an unusable answer rather than reporting every
path as dead — checked against a catalogue path that does not exist, it exits
non-zero instead of printing 126 false positives.