From 74e0c580d67fed47af74541c4c8afed8a4d16a73 Mon Sep 17 00:00:00 2001 From: eshulman2 Date: Sun, 19 Jul 2026 14:14:27 +0300 Subject: [PATCH 1/2] Add controller review skill --- .agents/skills/review-controller/SKILL.md | 22 ++++++ .../references/controller-review-checklist.md | 77 +++++++++++++++++++ 2 files changed, 99 insertions(+) create mode 100644 .agents/skills/review-controller/SKILL.md create mode 100644 .agents/skills/review-controller/references/controller-review-checklist.md diff --git a/.agents/skills/review-controller/SKILL.md b/.agents/skills/review-controller/SKILL.md new file mode 100644 index 000000000..12a10a00c --- /dev/null +++ b/.agents/skills/review-controller/SKILL.md @@ -0,0 +1,22 @@ +--- +name: review-controller +description: Review OpenStack Resource Controller controller/API PRs and patches. Use when reviewing new or changed ORC controllers, actuator/status logic, CRD API types, dependencies, import/adoption behavior, error handling, or KUTTL/controller tests, especially before responding to maintainer review comments. +--- + +# Review Controller + +Use this skill as a review workflow, not as an implementation recipe. Start from the diff and judge whether the controller is robust, convention-compatible, and sufficiently covered by KUTTL. + +## Workflow + +1. Read the changed API type, actuator, status writer, controller setup, generated files, examples, and KUTTL suites for the affected resource. +2. Compare the change to nearby controllers with the same pattern: simple immutable resources, resources with dependencies, mutable-field reconcilers, import/adoption, or extra API calls. +3. Read [references/controller-review-checklist.md](references/controller-review-checklist.md) before writing findings or making fixes. +4. Prioritize findings in this order: incorrect API contract, unsafe lifecycle/dependency behavior, broken reconciliation semantics, missing status mapping, weak KUTTL coverage, then style/convention issues. +5. Prefer concrete fixes over broad advice. If you change code, regenerate and run focused tests plus lint; run KUTTL when `E2E_OSCLOUDS` is available. + +## Review Output + +Lead with findings, ordered by severity, using file and line references. For each finding, explain the observable failure or maintainer convention it violates. Keep summaries secondary. + +When addressing existing review comments, map each comment to a code/test change or explicitly explain why no change is needed. diff --git a/.agents/skills/review-controller/references/controller-review-checklist.md b/.agents/skills/review-controller/references/controller-review-checklist.md new file mode 100644 index 000000000..5fb9103af --- /dev/null +++ b/.agents/skills/review-controller/references/controller-review-checklist.md @@ -0,0 +1,77 @@ +# ORC Controller Review Checklist + +This checklist is distilled from recurring `mandre` and `winiciusallan` review feedback on ORC controller and API PRs. + +## API Shape + +- Do not expose raw OpenStack IDs in spec references. If an ID is the resource's own creation ID, document that and add the linter exception inline. +- Check OpenStack API docs, OpenStack service schema/source, and gophercloud behavior before adding validation. Avoid over-restricting fields when OpenStack accepts empty strings, shorter values, spaces, numeric protocol aliases, or service-specific formats. +- Copy OpenStack constants and terminology verbatim unless the API contract has a clear reason to use a Kubernetes-facing name. +- Use resource-specific types when services enforce different rules. Avoid premature common types for fields like extra specs or metadata when Nova, Cinder, Neutron, etc. differ. +- For key-value lists with unique keys, use `+listType=map` and `+listMapKey=`. Add validation tests when CEL or list-map behavior is non-trivial. +- Prefer `int32` or `int64` for Kubernetes API integer fields. Do not introduce narrower integer types even when OpenStack has a smaller effective range. +- Optional spec fields should usually be pointers with `omitempty` when the controller must distinguish unspecified from zero, empty string, or `false`. Non-pointer optional fields can create API compatibility problems later. +- Keep unsupported fields out of the API/status until the controller can populate or reconcile them. A smaller robust controller is preferred over a broad half-implemented one. +- If a mutable OpenStack field is not implemented yet, mark it immutable or document the limitation. Do not imply mutability without reconciliation and tests. +- If fields are mutually exclusive or exactly one must be set, enforce that in API validation instead of relying on actuator checks. +- Treat generated clients and public API changes as compatibility surface. Avoid breaking type changes unless the release plan allows a major version bump. +- API comments must describe the actual field and status behavior. Generated docs should be regenerated, not hand-edited. + +## Actuator And Reconciliation + +- Separate OpenStack API calls into separate reconcilers. Do not hide secondary operations such as extra specs, ACLs, tags, or metadata inside `CreateResource` if they can fail independently and need drift reconciliation. +- If OpenStack supports a field only through a separate update API, create a separate reconciler rather than folding it into the generic update path. +- Name reconcilers by concern, for example `reconcileExtraSpecs` or `reconcileMetadata`, not generic names when there is only one concern. +- Return `nil` when a reconciler finds no work, matching existing reconcile functions. +- Before making a field mutable, prove the controller can converge for set, update, and clear/unset cases. If gophercloud cannot clear a value or OpenStack defaults make desired state unknowable, keep the field immutable and document why. +- Check nil, empty, defaulted, and environment-dependent values carefully. Avoid logic that constantly reconciles because OpenStack returns defaults that differ from an omitted spec field. +- For Neutron updates, pass the resource revision number where the API supports it. When deciding whether an update is needed, prefer the local `needsUpdate`/update-map pattern used by nearby controllers. +- For non-retryable OpenStack or gophercloud errors, wrap with `orcerrors.Terminal(...)` using `ConditionReasonInvalidConfiguration`; leave retryable errors retryable. +- If custom osclient or unmarshalling code works around a gophercloud gap, add a short TODO/comment with the upstream issue or expected removal condition. Prefer fixing or waiting for gophercloud when the workaround would be broad or fragile. +- Do not add osclient wrapper tests just because a wrapper exists. Unit-test custom parsing, custom API behavior, or non-trivial actuator logic; rely on KUTTL for ordinary gophercloud wrapper calls. +- Avoid unused helpers and commented-out test/code blocks. If deferred behavior is important, reference an issue or add a short explanatory comment. +- Interface assertions are useful as developer feedback, but they are not functional behavior. + +## Dependencies + +- Use ORC dependency mechanisms for references to other ORC resources; never resolve OpenStack resources directly from user-provided IDs. +- Filter APIs should use ORC refs for ORC-managed resources, for example `ProjectRef` or `PortRef`, rather than exposing OpenStack IDs or legacy aliases such as tenant IDs unless the resource API specifically needs them. +- Resolve dependencies only where needed and only after checking reconcile status. If a dependency may be unavailable, guard before dereferencing status IDs. +- When multiple dependencies are required, merge their `ReconcileStatus` values so Progressing can report all missing or unavailable dependencies, not only the first one encountered. +- When dependencies are mutable, prove finalizers on old dependencies are handled. If not covered, prefer making dependency fields immutable for the first implementation. +- Choose deletion guards deliberately. Some relationships, such as flavor-like or server-group-like references, may need ordering without preventing deletion; verify OpenStack semantics before using a deletion guard. +- Use current dependency predicates such as `orcv1alpha1.IsAvailable` where that is the local convention. + +## Status + +- Status should reflect what OpenStack returns. Do not over-validate or normalize observed status beyond deterministic ordering and comparable representations. +- Status validation should usually be more relaxed than spec validation. Avoid status regex/enums unless needed to keep kube-apiserver validation safe; otherwise return OpenStack values verbatim. +- Optional empty OpenStack values should usually be omitted from status unless the project has a convention for surfacing zero values. +- Every status field added by the API should have a status writer mapping and a deterministic test, or be removed/deferred. +- For imported resources, ensure the status is refreshed enough to report the actual OpenStack state. +- Watch for stale or misleading status fields on resources that progress asynchronously. If a status field can remain stale without another reconcile, either poll/resync appropriately or leave it out. + +## KUTTL And Tests + +- KUTTL is the main confidence surface for controller behavior. Prefer meaningful KUTTL coverage over excessive actuator unit tests. +- Create-minimal tests should assert both expected values and important absent fields. The resulting resource should be exactly what the controller promises. +- Create-full tests should set each supported spec field to a non-default value when the environment can support it, then assert the corresponding status. Use a different `spec.resource.name` than `metadata.name` to prove OpenStack name override behavior when the resource supports names. +- Update tests should prove mutable fields actually changed in OpenStack and can be cleared when supported. +- Import tests should exercise all supported filter fields and prove the imported object is the intended resource, not a similarly named trap resource. +- Adoption tests should cover the case where an existing OpenStack resource matches the managed spec and can be adopted, including supplied identifiers such as addresses when relevant. +- Use YAML status matchers for fixed scalar status. Reserve CEL for dynamic IDs, cross-resource equality, list membership/size, and absence checks. +- Avoid redundant CEL expressions that duplicate normal KUTTL YAML matching. Use CEL when normal matching cannot assert the condition, especially absence checks. +- Add API validation tests for complex CEL, list-map uniqueness, or exact OpenStack regex constraints. +- If a field cannot be reliably asserted in e2e because the backend is environment-specific, document that in the test or PR instead of adding a flaky assertion. +- If a test needs admin credentials for a field such as `adminStateUp`, use the admin cloud only where required and add a short comment explaining why. +- Follow current KUTTL naming conventions, including `00-create-resource.yaml` for creation steps and the established `*-update`, `*-import`, and dependency suite layouts. +- Use `resource: {}` in minimal create/update tests when the only required resource name can come from `metadata.name`. + +## Scope And PR Hygiene + +- Keep unrelated generated or packaging churn out of controller PRs unless the change requires it. +- Regenerate after API changes: CRD, OpenAPI, applyconfig/client artifacts, examples/docs as applicable. +- Check examples and samples for API field renames. +- Prefer small, robust first implementations. Leave broader field support, optimization, or service-specific enhancements for follow-up PRs when they would need substantial extra dependency or drift handling. +- Remove scaffolding leftovers, stale TODOs, boilerplate comments, `.gitkeep` files in populated test directories, and outdated downstream references. +- Use current project boilerplate in new files, including `Copyright The ORC Authors.` From 8a5a4a2a895bace9a839d16c9d126d9b5d3adc5f Mon Sep 17 00:00:00 2001 From: eshulman2 Date: Sun, 19 Jul 2026 14:18:52 +0300 Subject: [PATCH 2/2] Remove reviewer names from skill --- .../review-controller/references/controller-review-checklist.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.agents/skills/review-controller/references/controller-review-checklist.md b/.agents/skills/review-controller/references/controller-review-checklist.md index 5fb9103af..010f61422 100644 --- a/.agents/skills/review-controller/references/controller-review-checklist.md +++ b/.agents/skills/review-controller/references/controller-review-checklist.md @@ -1,6 +1,6 @@ # ORC Controller Review Checklist -This checklist is distilled from recurring `mandre` and `winiciusallan` review feedback on ORC controller and API PRs. +This checklist is distilled from recurring review feedback on ORC controller and API PRs. ## API Shape