Skip to content

feat(baremetal): list the servers carrying a tag, filtered by the API - #264

Draft
Denis-hamon wants to merge 24 commits into
ovh:feat/baremetal-logsfrom
Denis-hamon:feat/baremetal-list-by-tag
Draft

feat(baremetal): list the servers carrying a tag, filtered by the API#264
Denis-hamon wants to merge 24 commits into
ovh:feat/baremetal-logsfrom
Denis-hamon:feat/baremetal-list-by-tag

Conversation

@Denis-hamon

Copy link
Copy Markdown
Contributor

What this is

The first concrete thing the v2 catalogue buys, now that #262 can fetch its schemas and #263 embeds one.

ovhcloud baremetal list --tag Compliance=PCI-DSS
ovhcloud baremetal list --tag owner:EXISTS
ovhcloud baremetal list --tag owner:NEQ=Denis --tag Team:LIKE=infra%

IAM tags are how a fleet is organised — owner, Compliance, Team, LandingZone — and nothing here could ask for them. The generic --filter cannot: it runs over the columns of the table, after every server has been fetched, and the tags are not among them.

Why it crosses catalogues, and why that is safe

GET /v2/dedicated/server takes an iamTags parameter the v1 collection does not, so the narrowing happens on the API. But the v2 object is {id, iam} — there is no machine in it, so a table built from it would lose every column. The servers it names are therefore read on v1.

The obvious risk is that the two catalogues do not list the same fleet. Measured:

servers
GET /v1/dedicated/server 35
GET /v2/dedicated/server 35
in one and not the other 0, both ways

Without --tag, nothing changes: the same v1 collection as before, and a test asserts the v2 one is never called.

The syntax is one rule

key[:OPERATOR][=value], with no invented punctuation:

written means
owner=Denis EQ, which is the API's own default
owner:EXISTS set to anything
owner:NEQ=Denis set to something else
Project:LIKE=Proof% the API's pattern syntax, passed through untouched

The operator is spelled with the name the API uses, which is what lets a refusal list the ones that exist and completion offer them. It is read from the embedded schema (iam.resource.TagFilter.OperatorEnum) rather than transcribed: a list copied into Go stops being true in silence, which is what the game protocols of #256 cost.

Three refusals, each for a reason:

🛑 unknown tag operator "CONTAINS" in "owner:CONTAINS=Denis"; use one of EQ, EXISTS, ILIKE, LIKE, NEQ, NEXISTS
🛑 EXISTS asks whether "owner" is set at all, so it takes no value ("owner:EXISTS=Denis")
🛑 EQ needs something to compare "owner" against; write owner=<value>, or use EXISTS or NEXISTS to ask whether it is set

The last one matters most: silently turning a valueless comparison into EXISTS would answer a different question and look like it worked. Only the first = separates, so a value may contain another one and the wildcards LIKE uses.

--tag and --filter are not the same thing and both are honoured, in that order — the API narrows, then the table filters.

Measured against the real account

asked servers checked against
--tag Compliance=PCI-DSS 4 4 servers carry that tag, read outside the CLI
--tag owner:EXISTS 10 10 carry an owner key
--tag Project:LIKE=Proof% 1 the one server tagged Proof-of-Concept
no --tag 35 the whole fleet
--tag Compliance:NEQ=PCI-DSS 0 right: all four servers with that key carry the same value

Encoding checked the same way: a query whose JSON does not survive answers 400 Parameter iamTags isn't formatted correctly: failed to decode json, and a filter carrying a space answers 200 with an empty list — so the space made it through intact.

Gate

go build, go vet, make wasm, go test ./..., docgen — all clean.

16 tests (11 unit, 5 cobra) and 10 sabotages, 10 red, restoration checked by digest.

One of them was worth the trouble on its own. Removing url.QueryEscape from the query left the test green: url.QueryUnescape returns an unescaped string unchanged, so a round-trip assertion passes on a query that was never escaped at all. The test now refuses to see a raw {, }, ", [ or ] in the query, and the sabotage goes red. The count above is the corrected run.

Stacked on #263#262#261.

Denis-hamon and others added 3 commits August 18, 2026 17:02
…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>
IAM tags are how a fleet is organised — owner, Compliance, Team, LandingZone —
and nothing here could ask for them. The generic --filter cannot: it runs over
the columns of the table, after every server has been fetched, and the tags are
not among them.

    ovhcloud baremetal list --tag Compliance=PCI-DSS
    ovhcloud baremetal list --tag owner:EXISTS
    ovhcloud baremetal list --tag owner:NEQ=Denis --tag Team:LIKE=infra%

The v2 collection takes an iamTags parameter the v1 one does not, so the
narrowing happens on the API rather than after the fact. The servers it names
are then read on v1, which is the only route that answers with a machine: the
v2 object is {id, iam} and a table built from it would lose every column.
Measured on 20 August 2026, both catalogues list exactly the same 35 servers on
this account, so nothing is lost in the crossing. Without --tag nothing changes
and the v2 collection is never called.

The syntax is one rule, key[:OPERATOR][=value], with no invented punctuation.
The operator is spelled with the name the API uses, which means a refusal can
list the ones that exist and completion can offer them; it is read from the
embedded schema rather than transcribed, because a list copied into Go stops
being true in silence — what the game protocols of ovh#256 cost.

EXISTS and NEXISTS ask whether a key is set at all, so they take no value, and
a comparison with no value is refused rather than quietly turned into EXISTS:
that would answer a different question. Only the first "=" separates, so a value
may contain another one and the wildcards LIKE uses.

--tag and --filter are not the same thing and both are honoured, in that order.

Measured against the real account: Compliance=PCI-DSS returns 4 servers,
owner:EXISTS returns 10, no --tag returns 35, and each matches a reading of the
tag maps taken outside the CLI. NEQ on Compliance returns 0, which is right —
all four servers carrying that key carry the same value.

11 unit tests, 5 cobra tests, 10 sabotages, 10 red.

Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Denis-hamon and others added 5 commits August 20, 2026 16:14
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>
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>
@Denis-hamon
Denis-hamon changed the base branch from main to feat/baremetal-logs August 20, 2026 15:19
Denis-hamon and others added 11 commits August 20, 2026 22:45
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>
The operator was read from the FIRST colon of what was typed, so any key holding
one lost everything after it: `--tag ovh:default=true` parsed as key "ovh" with
operator "DEFAULT" and came back refused for an unknown operator — one nobody had
typed. The schema documents "ovh:" as the prefix of every tag OVHcloud computes
itself, so that was the whole computed namespace, and the error pointed at the
wrong thing.

It now reads the operator from the last colon, and only when what follows is an
operator the API declares.

That leaves a genuinely ambiguous case, and the CLI does not choose: after the
last colon, "owner:CONTAINS=Denis" is either a typo for an operator or a key
called "owner:CONTAINS". Reading it as a key would send it to the API, come back
with no servers, and print "nothing matches" — a wrong answer wearing the shape of
an answer. So it is refused, with both readings named and the operators listed. A
key that really does hold a colon is written with its operator spelled out, which
is unambiguous.

The completer had the same defect and a worse version of it: on a key named
"ovh:default" it offered "ovh:EQ", the operator glued to the first segment, which
no longer names the key at all — the CLI telling the operator to type something it
would then reject. It now offers only forms the parser accepts, and the test feeds
every suggestion back through the parser rather than asserting on a hardcoded
shape. That test is what caught the first version of this fix, whose completer
offered the bare "ovh:default" while the parser refused it.

Four tests, three sabotages, three reds.

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

Copy link
Copy Markdown
Contributor Author

Human review — Doubt

L1 · The key[:OPERATOR][=value] syntax
Dimension: Naming

Expected: The syntax sticks. Operators carry the API's own names (EQ, NEQ, LIKE, ILIKE, EXISTS, NEXISTS), read from the schema and never retyped.

Rendering pretty ugly with ASCII FORMAT , some '|' are not aligned

— Denis (Product Manager), through CLI sandbox review page

@Denis-hamon

Copy link
Copy Markdown
Contributor Author

Human review — Does not work

L3 · A key containing a colon
Dimension: Error message

Expected: A suffix that is not an operator produces a refusal explaining the ambiguity instead of guessing.

🛑 "ovh:default=true" is ambiguous: "default" is not one of EQ, EXISTS, ILIKE, LIKE, NEQ, NEXISTS, so this can only be read as the tag key "ovh:default" with no operator.
If that is what you meant, write the operator out: --tag ovh:default:EQ=
exit 1

— Denis (Product Manager), through CLI sandbox review page

@Denis-hamon

Copy link
Copy Markdown
Contributor Author

Two follow-ups from yesterday's human review.

L1 ("Rendering pretty ugly with ASCII FORMAT, some '|' are not aligned"): not a defect in this code. Replayed baremetal list --tag Compliance=PCI-DSS for real (binary built from this branch, live account) and captured the raw bytes — every data row is byte-identical in width to its neighbours, and the border rows match exactly:

327 ┌────────────────────────────┬──────────────────┬────────────────────────────┬──────────────────────┬───────┐
121 │            name            │      region      │        displayName         │          os          │ state │
327 ├────────────────────────────┼──────────────────┼────────────────────────────┼──────────────────────┼───────┤
121 │ ns537688.ip-139-99-131.net │ ap-southeast-syd │ ns537688.ip-139-99-131.net │ win2019-std_64       │ ok    │
...

The table renders correctly in a real terminal. The reviewer saw it through the review sandbox's web page (a separate, internal tool, not this repo): its embedded "IBM Plex Mono" web font is a 218-glyph Latin-only subset with no Unicode box-drawing coverage (checked its cmap directly — ─│┌┐└┘├┤┬┴┼ are all absent, plain ASCII |/- are present). The browser falls back per-glyph to a system font with different metrics for just the border characters, so drifts against the ASCII text beside it while the actual CLI output never does. Fixed in the sandbox itself today (rendering terminal output with a system monospace stack instead of the branded webfont) — unrelated repo, no change needed here.

L3 ("A key containing a colon" — --tag ovh:default=true): already matches the expected behaviour. splitTagFilter (internal/services/baremetal/tags.go) refuses it with the ambiguity spelled out, exactly as described:

🛑 "ovh:default=true" is ambiguous: "default" is not one of EQ, EXISTS, ILIKE,
   LIKE, NEQ, NEXISTS, so this can only be read as the tag key "ovh:default"
   with no operator.
   If that is what you meant, write the operator out: --tag ovh:default:EQ=<value>

and --tag ovh:default:EQ=true is accepted. Marking as working as intended.

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>
@Denis-hamon

Copy link
Copy Markdown
Contributor Author

Follow-up on L3 — a false negative.

🛑 "ovh:default=true" is ambiguous: "default" is not one of EQ, EXISTS, ILIKE, LIKE, NEQ, NEXISTS,
   so this can only be read as the tag key "ovh:default" with no operator.
   If that is what you meant, write the operator out: --tag ovh:default:EQ=<value>

That is the check's own expected result: "A suffix that is not an operator produces a refusal explaining the ambiguity instead of guessing." The command names the ambiguity, lists the operators, and gives the disambiguating form. Exit 1 is correct here.

The misreading came from the sandbox, which never said a check can be validated by a refusal — a 🛑 and a non-zero exit read as a failure, which is what a terminal teaches. Eight checks are in that case; they now carry the warning before the click, so the reviewer rules on the wording of the refusal rather than on the fact that there is one.

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>
@Denis-hamon

Copy link
Copy Markdown
Contributor Author

Human review — Doubt

L3 · A key containing a colon
Dimension: Error message

Expected: A suffix that is not an operator produces a refusal explaining the ambiguity instead of guessing.

🛑 "ovh:default=true" is ambiguous: "default" is not one of EQ, EXISTS, ILIKE, LIKE, NEQ, NEXISTS, so this can only be read as the tag key "ovh:default" with no operator.
If that is what you meant, write the operator out: --tag ovh:default:EQ=

— Denis (Product Manager), through CLI sandbox review page

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