mirror of
https://github.com/GoodOlClint/PSProxmoxVE.git
synced 2026-09-04 11:15:34 +00:00
goodolclint-claude[bot]
b90791e2bf
D001-D021 become ADR 0001-0021 in docs/decisions/, one decision per file. D017's PESTER_VERSION amendment was a second decision in one entry and becomes ADR 0022. ADR 0023 records the migration and reverses the lane2-change-plan ruling that deliberately kept DECISIONS.md until the CI lane work landed. DECISIONS.md is reduced to a stub with a D-to-ADR redirect table, so the four released CHANGELOG entries and older issue bodies that cite it degrade to a redirect rather than a dead reference. docs/review/ and docs/lane2-change-plan.md are deleted (ADR 0024). Of 91 findings, 83 were resolved and six of the seven still open were already GitHub issues; F021 was the exception and is now #130. CLAUDE.md's Key Conventions list gains the two rules it was missing and becomes the checklist, with the ADRs carrying rationale.
39 lines
1.8 KiB
Markdown
39 lines
1.8 KiB
Markdown
# ADR 0010 — VmId parameters are nullable int with ValidateRange
|
||
|
||
- **Status:** Accepted
|
||
- **Date:** 2026-03-22
|
||
- **Deciders:** unrecorded; adopted during review scans 2026-03-21 (ValidateRange) and 2026-03-22 (nullable)
|
||
- **Context source:** `docs/review/findings.json` F012, F038
|
||
|
||
## Context
|
||
|
||
PVE accepts VMIDs in the range 100–999999999. Without a `ValidateRange`, an out-of-range value reached the API and came back as a confusing server-side error rather than a parameter-binding failure the user could act on.
|
||
|
||
Separately, the firewall cmdlets operate at cluster, node or VM level, and used a non-nullable `int` defaulting to 0 for the optional VmId. That made "not specified" and "VM 0" the same value, so the cmdlet could not tell which scope the caller meant.
|
||
|
||
## Decision
|
||
|
||
- `[ValidateRange(100, 999999999)]` on every VmId parameter, mandatory or optional.
|
||
- `int?` when the parameter is optional, so absence is representable.
|
||
- `int` only when VmId is mandatory.
|
||
|
||
```csharp
|
||
[Parameter(Mandatory = true)]
|
||
[ValidateRange(100, 999999999)]
|
||
public int VmId { get; set; }
|
||
|
||
[Parameter()]
|
||
[ValidateRange(100, 999999999)]
|
||
public int? VmId { get; set; }
|
||
```
|
||
|
||
## Rejected alternatives
|
||
|
||
A non-nullable `int` for optional VmId, using 0 as the sentinel for "not supplied". Rejected because 0 is indistinguishable from a supplied value, and because it silently defeats `ValidateRange` — the default sits outside the valid range and never trips it.
|
||
|
||
## Consequences
|
||
|
||
The range check is on the parameter, not in the service, so a service called directly from another service is not covered by it.
|
||
|
||
`Get-PveTaskList` was found later still missing the attribute on its optional `int?` VmId, which is the failure mode this rule invites: adding `int?` is the visible half and it is easy to stop there.
|