fix(edit): keep flags explicitly set to their zero value

[Denis-hamon]

Updated 2026-10-01. As asked in review, there is now an end-to-end test on a call site this PR had to change, not only on one that already passed a pointer. vps edit --sla-monitoring=false must send "slaMonitoring": false. The test fails when VpsSpec is passed by value again and passes with &VpsSpec. The sentence on what is preserved is also corrected below: the flag's own value overwrites whatever the body held at that path, and siblings are what is kept.

---

Description

Edit commands marshal their parameters struct, which uses omitempty. A flag set to false, 0 or "" therefore disappears from the request body. That body is then merged into the resource fetched from the API — which restores the remote value. The call succeeds, nothing changes, and the CLI prints ✅ Resource updated successfully.

Concretely, on a dedicated server:

$ ovhcloud baremetal edit ns1234.ip-11.22.33.net --monitoring=false
✅ Resource updated successfully      # monitoring is still enabled

The operator finds out during the maintenance window they thought they had silenced.

This PR puts explicitly-set values back into the body before the merge.

Flags are matched to struct fields by address, not by name. pflag keeps the pointer it was given (type boolValue bool, newBoolValue(val, p) returns (*boolValue)(p)), so a flag value and the field it writes to share an address. Matching by name would have silently missed the commands that expose a field under an unrelated flag name — PrivateNetworkRoutingAsDefault behind --routing-as-default, or nested flags using dots such as --network.private.create.subnet-enable-dhcp — that is, it would have reproduced the same bug elsewhere.

Nested structs are supported, values already present next to the restored field are preserved (the field itself takes the flag's value), and fields the user did not mention stay out: an edit remains a partial update.

Correction — the reach of the first commit was overstated

The automated review was right, and I had it wrong. The first commit claimed every edit command benefited. They did not.

Matching by address only works on the struct the flags were registered against. EditResource received a copy from every service but BareMetal, and a copy holds none of those addresses, so the guard clause on a non-pointer argument returned immediately. The mechanism was a silent no-op everywhere else — it fixed nothing and reported nothing, which is the worst of the two.

Measured on this tree before the second commit, rebased on main at bdfc23b:

call sites of EditResource : 71
  by pointer (fix applies)  :  1   -> baremetal
  by value   (no-op)        : 70

The second commit hands the struct over by pointer at 63 further call sites. EditResource only marshals that argument to JSON, and a pointer to a struct marshals exactly like the struct, so nothing else changes.

Seven call sites keep their current argument, each for its own reason:

call site	argument	why
vrackServices	nil	no editable parameter
container registry, Swift container, private database	map literal	every key is present by construction, omitempty cannot drop anything
container registry OIDC	payload	already assembles its body from Flags().Changed
managed database, managed analytics	userSpec	a per-engine struct rebuilt locally, so no flag points into the value being sent

The last two keep the defect, on the name and roles of a database user. Matching by address cannot reach them, and I would rather leave that visible than paper over it.

Rebased on main (bdfc23b, 2026-10-01). Main gained seven edit call sites since the first version of this PR. One of them, EditShareSnapshot from #284, handed its struct over by value; the call-site test failed on it after the rebase, naming the line, and the second commit now covers it. The managed-database site from #283 keeps its new networkFields argument and passes its struct by pointer.

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Improvement (improvement of existing commands)
• Breaking change (fix or feature that can break a current behavior)
• Documentation update

Checklist:

• My code follows the style guidelines of this project
• I have commented my code
• I ran go mod tidy
• I have added tests that prove my fix is effective or that my feature works

Six unit tests in internal/services/common/explicit_flags_test.go — false boolean kept, untouched fields left out, zero values of every kind, nested field reached through an unrelated flag name, siblings preserved, nil inputs tolerated — plus one end-to-end test asserting that baremetal edit --monitoring=false sends "monitoring": false in the PUT body.

explicit_flags_callsites_test.go walks the services tree and fails on any call site handing over a value, naming the file, the line and the fix. It carries a positive control: reverting a single site to its previous form makes it fail, so it gates the behaviour rather than its own presence. It also refuses to pass on an empty walk.

go build ./..., GOOS=js GOARCH=wasm go build ./..., go vet ./... and the full go test ./... pass. make doc produces no change: the behaviour of existing flags is fixed, not their surface.

[Copilot]

Pull request overview

Fixes edit commands so explicitly set zero values survive omitempty serialization.

Changes:

• Adds address-based flag-to-field restoration.
• Integrates restoration into EditResource.
• Adds unit and baremetal integration tests.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
internal/services/common/explicit_flags.go	Adds explicit zero-value restoration.
internal/services/common/explicit_flags_test.go	Tests restoration behavior.
internal/services/common/common.go	Integrates restoration into edits.
internal/cmd/baremetal_test.go	Tests disabling monitoring end-to-end.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Denis-hamon]

Confirmed, and fixed in 367aff1 → cc4f6a8.

I measured it on this tree before changing anything: of the 64 EditResource call sites, exactly one passed a pointer — baremetal, the one I wrote the patch and the test against. The other 63 handed over a copy, the guard clause returned immediately, and the mechanism did nothing at all. Silent, which is worse than broken.

56 call sites now pass a pointer. EditResource only marshals that argument to JSON, and a pointer to a struct marshals exactly like the struct, so nothing else moves.

Seven keep their current argument: nil for vrackServices, three map literals where every key is present by construction, the container registry OIDC payload which already reads Flags().Changed, and the two managed-database user specs rebuilt per engine — those two keep the defect, and matching by address cannot reach them.

Added explicit_flags_callsites_test.go, which walks the services tree and fails on any call site handing over a value, naming the file and the line. Reverting one site makes it fail, so it gates the behaviour and not its own presence.

[Denis-hamon]

Fixed: the value-style callers now pass an address, and
explicit_flags_callsites_test.go walks the AST of every EditResource call
site and fails on any that passes a struct by value. One call site that this
test catches only once the stack is assembled (internal/services/ip/admin.go,
from #257) has just been fixed on that branch.

[Denis-hamon]

@copilot Perform the review again on the two commits pushed since your last pass: the call sites now hand the parameters struct over by pointer, and a new AST test guards them. Golang files only.