diff --git a/.coderabbit.yaml b/.coderabbit.yaml index f2e7050609e0..66ea63486404 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -50,6 +50,100 @@ reviews: # Test fixtures and data - "!**/testdata/**" + path_instructions: + # --- Rules derived from PR review patterns --- + # Generated by teams:coderabbit-rules-from-pr-reviews (2026-03-27) + # Analyzed 100 recent merged PRs, 480 human review comments from 24 reviewers + + - path: "api/**/*_types.go" + instructions: | + Do not use +kubebuilder:validation:Pattern for string validation. Use CEL + XValidation rules instead with a human-readable message explaining the + expected format. For example, instead of: + +kubebuilder:validation:Pattern=`^arn:` + Use: + +kubebuilder:validation:XValidation:rule="self.matches('^arn:...')",message="must be a valid ARN in the format arn::..." + + Enum values in kubebuilder:validation:Enum markers must use PascalCase + (e.g., Standard, IO1, GP2) following Kubernetes API conventions. Avoid + lowercase (e.g., "enabled"/"disabled") or camelCase enum values. + Also prefer specific descriptive terms over generic Enable/Disable + (e.g., HTTPEndpoint/None instead of Enabled/Disabled). + + Every API field with constrained values (enums, formatted strings, ranges) + must document in the godoc comment: + 1. The list of valid values and what each means to an end user + 2. The expected format for string fields (e.g., "must be a valid IPv4 or IPv6 address") + 3. The behavior when the field is omitted (e.g., "When omitted, the platform + is left to choose a reasonable default, which is subject to change over time. + The current default is ...") + Do not assume users will read kubebuilder markers — the godoc should be + self-contained. + + Follow OpenShift API conventions for pointer usage: + - Do NOT use pointers for struct fields when the zero value is not a valid + user choice. Use value types with omitzero instead. + - Do NOT use pointers for enum fields unless empty string "" is a valid enum + value. + - Do NOT use pointers for numeric fields when the minimum value is > 0. + - Use omitzero (not omitempty) for struct value types — omitempty does + nothing for non-pointer structs. + - Only use pointers when you need to distinguish between "not set" and + "explicitly set to zero value" and both are valid user choices. + See: https://github.com/openshift/enhancements/blob/master/dev-guide/api-conventions.md + + All string fields in API types must have both MinLength and MaxLength + kubebuilder validation markers unless there is a documented reason not to. + Common values: + - IPv4/IPv6 addresses: MinLength=3, MaxLength=39 + - DNS names: MinLength=1, MaxLength=253 + - Kubernetes resource names: MinLength=1, MaxLength=253 + Empty string is rarely a valid value — if not, set MinLength=1 at minimum. + + When a struct has mutually exclusive sub-structs (e.g., platform-specific + config), use a properly marked discriminated union: + 1. Add +union marker to the parent struct + 2. Add +unionDiscriminator to the discriminator field + 3. Add +unionMember to each member field + 4. Add bidirectional CEL validation that enforces "required when discriminator + matches AND forbidden otherwise": + rule="self.type == 'Foo' ? has(self.foo) : !has(self.foo)" + 5. Consider placing the union in a child struct to keep the parent clean for + future non-union fields. + Do not use separate has() rules without the forbidden-otherwise check. + + Do not import and embed upstream Kubernetes types (e.g., + corev1.LocalObjectReference, corev1.SecretReference) in CRD API types. + Create your own minimal reference types instead. Upstream types can change + unexpectedly or add fields you don't want to support, breaking your API + contract. Only import upstream types when passing them through verbatim to + another Kubernetes API. + + Do not use +patchStrategy or +patchMergeKey markers on CRD types — they + have no effect. CRDs use Server-Side Apply (SSA) with +listType=map and + +listMapKey markers instead for merge semantics on list fields. + + - path: "**/*_test.go" + instructions: | + Test case names should follow the Gherkin convention pattern: + "When , it should ". + For example: "When cluster has no nodes, it should clear NodesInfo status" + This makes test output self-documenting and easier to understand in CI logs. + + - path: "**/*.go" + instructions: | + When writing data to files or performing operations where order matters, + prefer slices of structs over maps to ensure deterministic iteration order. + Go map iteration is randomized and can cause flaky tests or inconsistent + output. + + New exported functions and non-trivial logic changes should include unit + tests. Tests should cover: + 1. Happy path + 2. Error/edge cases (malformed input, empty values, nil) + 3. Platform-specific behavior for all relevant platforms (e.g., if adding + AWS support, consider whether GCP/Azure also need test coverage) + Check that corresponding _test.go files are included in the PR. tools: golangci-lint: enabled: true