This PR introduces a shared named enum feature to the schema generator, allowing multiple entity types in the same domain to share a single strongly-typed Go enum instead of each declaring its own distinct string type. The motivation — avoiding verbose switch blocks to translate between what were logically identical enum values — is well-founded, and the design is sound.
What the change does, in brief:
enums: section to each schema YAML. Each named enum declares its values and a physical encoding (ref, string, or keyword).RestartPolicy, PortProtocol) and constants (RestartPolicyAlways, etc.) at the top of the generated file.SandboxSpecRestartPolicy, PortProtocol per-component) now use a Go type alias (= RestartPolicy) pointing at the shared type, and keep their own lookup maps with the correct per-field entity IDs.SCHEDULABLE, PENDING, etc.) remain as aliases pointing at the new canonical constants, preserving call-site compatibility.SchemaField gains Enum, EnumEncoding, and EnumMembers fields; EnumValue/EnumMember helpers cover ref, string, and keyword encodings with explicit backward-compat handling for older schemas that only carried EnumValues.choices: attribute is fully banned at the schema layer now — validation rejects it; all callers in the YAML files are already migrated to enum: + encoding: ref.What I checked:
Backward compatibility of stored entities. The PR title says "without changing stored entities", and that holds up. Every existing entity continues to store enum fields as the same entity ref IDs it always did (e.g. dev.miren.compute/status.running). The new canonical constant values are the natural-language strings ("running"), and the per-field FromId/ToId maps still do the same ref↔string translation they always did. No migration of stored data is needed.
choices: ban completeness. The search for choices: in any YAML file comes back empty — every schema file has been migrated.
Validator extension. The new enum-constraint checking in validation.go is added for TypeKeyword and TypeStr in the correct place (after type coercion). The existing TypeRef + TypeEnum paths are unchanged. The exempt predicate is correctly threaded through for update semantics.
Generator validation. validateEnums checks for empty names, empty value lists, duplicate values, name collisions with generated struct names, and requires every enum-typed field to reference a known named enum. That's solid defensive coverage.
Tests. api/core/core_v1alpha/named_enum_test.go verifies that the shared type encodes correctly through two different field paths (with distinct entity IDs). pkg/entity/natural_test.go, pkg/model/enum_test.go, and the generator tests are all extended appropriately.
One small gap worth noting: validateEnums (generator.go lines 359-369) validates that enum-typed fields referencing a named enum also have a valid encoding. But the encoding field only exists on schemaAttr, not on the named enum definition in schemaEnum. If a developer forgets to specify encoding on a field, the validator catches it, which is the right place. The one thing that's currently unchecked is whether the enum type's intrinsic values are consistent with the encoding (e.g., no structural constraint prevents a keyword-encoded enum from having values that wouldn't form valid keywords). This is a minor gap and not a blocker.
Storage schema. api/storage/storage_v1alpha/schema.go is handwritten (not generated) and has been manually updated following the same pattern. The new tests in named_enum_test.go cover the critical encode/decode round-trip.
Overall the implementation is cohesive, the migration is mechanical and consistent across all six API domains, and the tests cover the meaningful invariants. This is ready for human review to merge.
Verdict: ready
{
"owner": "mirendev",
"repo": "runtime",
"number": 1113,
"verdict": "ready",
"event": "comment",
"summary": "This PR introduces a **shared named enum** feature to the schema generator, allowing multiple entity types in the same domain to share a single strongly-typed Go enum instead of each declaring its own distinct `string` type. The motivation — avoiding verbose `switch` blocks to translate between what were logically identical enum values — is well-founded, and the design is sound.\n\n**What the change does, in brief:**\n- Adds a top-level `enums:` section to each schema YAML. Each named enum declares its values and a physical encoding (`ref`, `string`, or `keyword`).\n- The generator emits a single canonical Go type (e.g. `RestartPolicy`, `PortProtocol`) and constants (`RestartPolicyAlways`, etc.) at the top of the generated file.\n- Individual fields that previously each declared their own concrete type (e.g. `SandboxSpecRestartPolicy`, `PortProtocol` per-component) now use a Go type alias (`= RestartPolicy`) pointing at the shared type, and keep their own lookup maps with the correct per-field entity IDs.\n- Old all-caps legacy constants (`SCHEDULABLE`, `PENDING`, etc.) remain as aliases pointing at the new canonical constants, preserving call-site compatibility.\n- `SchemaField` gains `Enum`, `EnumEncoding`, and `EnumMembers` fields; `EnumValue`/`EnumMember` helpers cover ref, string, and keyword encodings with explicit backward-compat handling for older schemas that only carried `EnumValues`.\n- The `choices:` attribute is **fully banned** at the schema layer now — validation rejects it; all callers in the YAML files are already migrated to `enum:` + `encoding: ref`.\n\n**What I checked:**\n\n*Backward compatibility of stored entities.* The PR title says \"without changing stored entities\", and that holds up. Every existing entity continues to store enum fields as the same entity ref IDs it always did (e.g. `dev.miren.compute/status.running`). The new canonical constant values are the natural-language strings (`\"running\"`), and the per-field `FromId`/`ToId` maps still do the same ref↔string translation they always did. No migration of stored data is needed.\n\n*`choices:` ban completeness.* The search for `choices:` in any YAML file comes back empty — every schema file has been migrated.\n\n*Validator extension.* The new enum-constraint checking in `validation.go` is added for `TypeKeyword` and `TypeStr` in the correct place (after type coercion). The existing `TypeRef` + `TypeEnum` paths are unchanged. The `exempt` predicate is correctly threaded through for update semantics.\n\n*Generator validation.* `validateEnums` checks for empty names, empty value lists, duplicate values, name collisions with generated struct names, and requires every `enum`-typed field to reference a known named enum. That's solid defensive coverage.\n\n*Tests.* `api/core/core_v1alpha/named_enum_test.go` verifies that the shared type encodes correctly through two different field paths (with distinct entity IDs). `pkg/entity/natural_test.go`, `pkg/model/enum_test.go`, and the generator tests are all extended appropriately.\n\n*One small gap worth noting:* `validateEnums` (generator.go lines 359-369) validates that `enum`-typed fields referencing a named enum also have a valid `encoding`. But the `encoding` field only exists on `schemaAttr`, not on the named enum definition in `schemaEnum`. If a developer forgets to specify `encoding` on a field, the validator catches it, which is the right place. The one thing that's currently unchecked is whether the `enum` type's intrinsic `values` are consistent with the encoding (e.g., no structural constraint prevents a keyword-encoded enum from having values that wouldn't form valid keywords). This is a minor gap and not a blocker.\n\n*Storage schema.* `api/storage/storage_v1alpha/schema.go` is handwritten (not generated) and has been manually updated following the same pattern. The new tests in `named_enum_test.go` cover the critical encode/decode round-trip.\n\nOverall the implementation is cohesive, the migration is mechanical and consistent across all six API domains, and the tests cover the meaningful invariants. This is ready for human review to merge.",
"posted_to_pr": true,
"draft": true
}